Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

developments for postgresql v14 #104

Merged
merged 11 commits into from
Jun 11, 2021
Merged

Conversation

mikecaat
Copy link
Contributor

@mikecaat mikecaat commented Jun 1, 2021

TODO

  • need review
  • update nbtsort-14.c again for the newer v14 version
  • check release notes

This patch handles the change of PostgreSQL core's
67a472d7. It corresponds the change of simple_prompt() interface
and removes arbitrary restrictions on password length.
PostgreSQL core code has the same function name, but the
implementation is not same. So, rename from appendStringInfoVA()
to appendStringInfoVA_c().
This patch handles the changes of PostgreSQL core's
9626325d. It adds heuristic incoming-message-size
limits in the server.
Fix TupleParserInit() and TupleParseDumpRecord()
This patch handles the following PostgreSQL core's changes.

1. The patch(1375422c) removes es_result_relations and
es_num_result_relations to simplify things.

2. The patch(a04daa97) removes es_result_relation_info
to improve maintainability.
To handle the PostgreSQL's patch(a3dc9260), which refactors
option handling of REINDEX.
@mikecaat mikecaat mentioned this pull request Jun 4, 2021
Copy link
Contributor

@MoonInsung MoonInsung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @mikecaat .

Thank you for responding to the PostgreSQL 14.

First, I used this PR in my environment(CentOS7/x86) to
build tests and regression tests from PG9.6 to the master branch,
and there were no problems.

Looking at this patch, I'm a little shocked that so far,
I haven't had any issues with e842847, d8ceb1a, and 969896c.
Thank you so much for catching these issues and correcting them.

And I checked each commit, but there are no comments other than 9b56a46.
I'll comment on 9b56a46 in line with the source code.

Best regards.
Moon.

* create and initialize a spool structure
*/
static BTSpool *
_bt_spoolinit(Relation heap, Relation index, bool isunique, bool isdead)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why your opinion moved the "_bt_spoolinit" function outside?
In my think, it's more consistent with adding to "nbtsort-14.c" like what we've worked on it so far.
If you think it is good to add the "_bt_spoolinit" function to "nbtsort.c
", I think it would be good to move all "_bt_spoolinit" functions included to "nbtsort-XX.c" to "nbtsort.c".

What do you think of this opinion?

Copy link
Contributor Author

@mikecaat mikecaat Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it's better for a maintenance reason.

I puzzled that the header comments of "nbtsort-XX.c" say the identification is "src/backend/access/nbtree/nbtsort.c", but there is a fucntion which the PostgreSQL doesn't have in the version.

I though it's consistent if "nbtsort-XX.c" is just copy of postgresql's "src/backend/access/nbtree/nbtsort.c".

If you think it is good to add the "_bt_spoolinit" function to "nbtsort.c
", I think it would be good to move all "_bt_spoolinit" functions included to "nbtsort-XX.c" to "nbtsort.c".

OK, but why don't you after merging this commit? Because _bt_spoolinit was PostgreSQL's code, for example, 9.6 has that. After merging this, I'll update all nbtsort-XX.c to all stable PostgreSQL version's nbtsort.c and if there are other functions for only pg_bulkload, I'll move to "nbtsort.c" in pg_bulkload together.

BTW, should "nbtsort.c" in pg_bulkload be renamed though I didn't come up with any good name? This makes maintainers confused, doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. For that reason, I agree with your opinion.

OK, but why don't you after merging this commit?

Also, I understand moving the "_bt_spoolinit" funciton and
anyone from "nbtsort-xx.c" to "nbtsort.c" after committing the current PR.

BTW, should "nbtsort.c" in pg_bulkload be renamed though I didn't come up with any good name? This makes maintainers confused, doesn't it?

I think that "nbtsort-common.c" might be good
because this file includes a common functions.
Of course, this is my personal opinion, so I think it's fine as it is. :)

Best regards.
Moon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, OK

Also, I understand moving the "_bt_spoolinit" funciton and
anyone from "nbtsort-xx.c" to "nbtsort.c" after committing the current PR.

I made #106.

I think that "nbtsort-common.c" might be good
because this file includes a common functions.
Of course, this is my personal opinion, so I think it's fine as it is. :)

It looks good to me. I changed the name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the file name and creating a new issue!
I think there's nothing problme with committing this PR.
That is, it looks good to me.

@MoonInsung
Copy link
Contributor

If there is no other opinion, I'll commit this PR around 15:00(JST) on June 11.

Best regards.
Moon.

@MoonInsung MoonInsung merged commit d3f699a into ossc-db:master Jun 11, 2021
mikecaat added a commit to mikecaat/pg_bulkload that referenced this pull request Nov 11, 2021
* Fix prompt_for_password() against PostgreSQL v14 Beta1

This patch handles the change of PostgreSQL core's
67a472d7. It corresponds the change of simple_prompt() interface
and removes arbitrary restrictions on password length.

* Rename from appendStringInfoVA() to appendStringInfoVA_c()

PostgreSQL core code has the same function name, but the
implementation is not same. So, rename from appendStringInfoVA()
to appendStringInfoVA_c().

* Fix the wrong return type of *WriterInsertProc

* Reuse the RelationSetNewRelfilenode's definition for v14 beta1

* Fix pg_getmessage arguments against PostgreSQL v14 Beta1

This patch handles the changes of PostgreSQL core's
9626325d. It adds heuristic incoming-message-size
limits in the server.

* Fix the wrong arguments types of functions

Fix TupleParserInit() and TupleParseDumpRecord()

* Fix EState struct againt PostgreSQL v14 Beta1

This patch handles the following PostgreSQL core's changes.

1. The patch(1375422c) removes es_result_relations and
es_num_result_relations to simplify things.

2. The patch(a04daa97) removes es_result_relation_info
to improve maintainability.

* Fix argments of reindex_index() againt PostgreSQL v14 Beta1

To handle the PostgreSQL's patch(a3dc9260), which refactors
option handling of REINDEX.

* Update nbtsort.c against PostgreSQL v14 Beta1

* Update test codes for PostgreSQL v14 Beta1

* Update the github action workflow for PostgreSQL14 Beta1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants