Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers
From | Shubham Khanna |
---|---|
Subject | Re: Pgoutput not capturing the generated columns |
Date | |
Msg-id | CAHv8RjLLy=otO_c0y5FN2jipajC2J6+ZRaTcWmEv4X1kmzAjtw@mail.gmail.com Whole thread Raw |
In response to | Re: Pgoutput not capturing the generated columns (Peter Smith <smithpb2250@gmail.com>) |
Responses |
Re: Pgoutput not capturing the generated columns
Re: Pgoutput not capturing the generated columns |
List | pgsql-hackers |
On Thu, Jun 27, 2024 at 2:41 PM Peter Smith <smithpb2250@gmail.com> wrote: > > Hi, here are some patch v11-0001 comments. > > (BTW, I had difficulty reviewing this because something seemed strange > with the changes this patch made to the test_decoding tests). > > ====== > General > > 1. Patch name > > Patch name does not need to quote 'logical replication' > > ~ > > 2. test_decoding tests > > Multiple test_decoding tests were failing for me. There is something > very suspicious about the unexplained changes the patch made to the > expected "binary.out" and "decoding_into_rel.out" etc. I REVERTED all > those changes in my nitpicks top-up to get everything working. Please > re-confirm that all the test_decoding tests are OK! > > ====== > Commit Message > > 3. > Since you are including the example usage for test_decoding, I think > it's better to include the example usage of CREATE SUBSCRIPTION also. > > ====== > contrib/test_decoding/expected/binary.out > > 4. > SELECT 'init' FROM > pg_create_logical_replication_slot('regression_slot', > 'test_decoding'); > - ?column? > ----------- > - init > -(1 row) > - > +ERROR: replication slot "regression_slot" already exists > > Huh? Why is this unrelated expected output changed by this patch? > > The test_decoding test fails for me unless I REVERT this change!! See > my nitpicks diff. > > ====== > .../expected/decoding_into_rel.out > > 5. > -SELECT 'stop' FROM pg_drop_replication_slot('regression_slot'); > - ?column? > ----------- > - stop > -(1 row) > - > > Huh? Why is this unrelated expected output changed by this patch? > > The test_decoding test fails for me unless I REVERT this change!! See > my nitpicks diff. > > ====== > .../test_decoding/sql/decoding_into_rel.sql > > 6. > -SELECT 'stop' FROM pg_drop_replication_slot('regression_slot'); > +SELECT 'stop' FROM pg_drop_replication_slot('regression_slot'); > > Huh, Why does this patch change this code at all? I REVERTED this > change. See my nitpicks diff. > > ====== > .../test_decoding/sql/generated_columns.sql > > (see my nitpicks replacement file for this test) > > 7. > +-- test that we can insert the result of a 'include_generated_columns' > +-- into the tables created. That's really not a good idea in practical terms, > +-- but provides a nice test. > > NITPICK - I didn't understand the point of this comment. I updated > the comment according to my understanding. > > ~ > > NITPICK - The comment "when 'include-generated-columns' is not set > then columns will not be replicated" is the opposite of what the > result is. I changed this comment. > > NITPICK - modified and unified wording of some of the other comments > > NITPICK - changed some blank lines > > ====== > contrib/test_decoding/test_decoding.c > > 8. > + else if (strcmp(elem->defname, "include-generated-columns") == 0) > + { > + if (elem->arg == NULL) > + data->include_generated_columns = true; > > Is there any way to test that "elem->arg == NULL" in the > generated.sql? OTOH, if it is not possible to get here then is the > code even needed? > Currently I could not find a case where the 'include_generated_columns' option is not specifying any value, but I was hesitant to remove this from here as the other options mentioned follow the same rules. Thoughts? > ====== > doc/src/sgml/ddl.sgml > > 9. > <para> > - Generated columns are skipped for logical replication and cannot be > - specified in a <command>CREATE PUBLICATION</command> column list. > + 'include_generated_columns' option controls whether generated columns > + should be included in the string representation of tuples during > + logical decoding in PostgreSQL. The default is <literal>true</literal>. > </para> > > NITPICK - Use proper markdown instead of single quotes for the parameter. > > NITPICK - I think this can be reworded slightly to provide a > cross-reference to the CREATE SUBSCRIPTION parameter for more details > (which means then we can avoid repeating details like the default > value here). PSA my nitpicks diff for an example of how I thought docs > should look. > > ====== > doc/src/sgml/protocol.sgml > > 10. > + The default is true. > > No, it isn't. AFAIK you made the default behaviour true only for > 'test_decoding', but the default for CREATE SUBSCRIPTION remains > *false* because that is the existing PG17 behaviour. And the default > for the 'include_generated_columns' in the protocol is *also* false to > match the CREATE SUBSCRIPTION default. > > e.g. libpqwalreceiver.c only sets ", include_generated_columns 'true'" > when options->proto.logical.include_generated_columns > e.g. worker.c says: options->proto.logical.include_generated_columns = > MySubscription->includegencols; > e.g. subscriptioncmds.c sets default: opts->include_generated_columns = false; > > (This confirmed my previous review expectation that using different > default behaviours for test_decoding and pgoutput would surely lead to > confusion) > > ~~~ > > 11. > - <para> > - Next, the following message part appears for each column included in > - the publication (except generated columns): > - </para> > - > > AFAIK you cannot just remove this entire paragraph because I thought > it was still relevant to talking about "... the following message > part". But, if you don't want to explain and cross-reference about > 'include_generated_columns' then maybe it is OK just to remove the > "(except generated columns)" part? > > ====== > src/test/subscription/t/011_generated.pl > > NITPICK - comment typo /tab2/tab3/ > NITPICK - remove some blank lines > > ~~~ > > 12. > # the column was NOT replicated (the result value of 'b' is the > subscriber-side computed value) > > NITPICK - I think this comment is wrong for the tab2 test because here > col 'b' IS replicated. I have added much more substantial test case > comments in the attached nitpicks diff. PSA. > > ====== > src/test/subscription/t/031_column_list.pl > > 13. > NITPICK - IMO there is a missing word "list" in the comment. This bug > existed already on HEAD but since this patch is modifying this comment > I think we can also fix this in passing. All the comments are handled. The attached Patches contain all the suggested changes. Thanks and Regards, Shubham Khanna.
Attachment
pgsql-hackers by date: