Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers
From | Peter Smith |
---|---|
Subject | Re: Pgoutput not capturing the generated columns |
Date | |
Msg-id | CAHut+PsJvLTXfo_GcRz+e+s_KC8nEPY5xtTt-39cKUvfs=MsJQ@mail.gmail.com Whole thread Raw |
In response to | Re: Pgoutput not capturing the generated columns (Shubham Khanna <khannashubham1197@gmail.com>) |
Responses |
Re: Pgoutput not capturing the generated columns
|
List | pgsql-hackers |
Hi, Here are my review comments for patch v22-0001 All comments now are only for the TAP test. ====== src/test/subscription/t/011_generated.pl 1. I added all new code for the missing combination test case "gen-to-missing". See nitpicks diff. - create a separate publication for this "tab_gen_to_missing" table because the test gives subscription errors. - for the initial data - for the replicated data ~~~ 2. I added sub1 and sub2 subscriptions for every combo test (previously some were absent). See nitpicks diff. ~~~ 3. There was a missing test case for nogen-to-gen combination, and after experimenting with this I am getting a bit suspicious, Currently, it seems that if a COPY is attempted then the error would be like this: 2024-08-01 17:16:45.110 AEST [18942] ERROR: column "b" is a generated column 2024-08-01 17:16:45.110 AEST [18942] DETAIL: Generated columns cannot be used in COPY. OTOH, if a COPY is not attempted (e.g. copy_data = false) then patch 0001 allows replication to happen. And the generated value of the subscriber "b" takes precedence. I have included these tests in the nitpicks diff of patch 0001. Those results weren't exactly what I was expecting. That is why it is so important to include *every* test combination in these TAP tests -- because unless we know how it works today, we won't know if we are accidentally breaking the current behaviour with the other (0002, 0003) patches. Please experiment in patches 0001 and 0002 using tab_nogen_to_gen more to make sure the (new?) patch errors make sense and don't overstep by giving ERRORs when they should not. ~~~~ Also, many other smaller issues/changes were done: ~~~ Creating tables: nitpick - rearranged to keep all combo test SQLs in a consistent order throughout this file 1/ gen-to-gen 2/ gen-to-nogen 3/ gen-to-missing 4/ missing-to-gen 5/ nogen-to-gen nitpick - fixed the wrong comment for CREATE TABLE tab_nogen_to_gen. nitpick - tweaked some CREATE TABLE comments for consistency. nitpick - in the v22 patch many of the generated col 'b' use different computations for every test. It makes it unnecessarily difficult to read/review the expected results. So, I've made them all the same. Now computation is "a * 2" on the publisher side, and "a * 22" on the subscriber side. ~~~ Creating Publications and Subscriptions: nitpick - added comment for all the CREATE PUBLICATION nitpick - added comment for all the CREATE SUBSCRIPTION nitpick - I moved the note about copy_data = false to where all the node_subscriber2 subscriptions are created. Also, don't explicitly refer to "patch 000" in the comment, because that will not make any sense after getting pushed. nitpick - I changed many subscriber names to consistently use "sub1" or "sub2" within the name (this is the visual cue of which node_subscriber<n> they are on). e.g. /regress_sub_combo2/regress_sub2_combo/ ~~~ Initial Sync tests: nitpick - not sure if it is possible to do the initial data tests for "nogen_to_gen" in the normal place. For now, it is just replaced by a comment. NOTE - Maybe this should be refactored later to put all the initial data checks in one place. I'll think about this point more in the next review. ~~~ nitpick - Changed cleanup I drop subscriptions before publications. nitpick - remove the unnecessary blank line at the end. ====== Please see the attached diffs patch (apply it atop patch 0001) which includes all the nipick changes mentioned above. ~~ BTW, For a quicker turnaround and less churning please consider just posting the v23-0001 by itself instead of waiting to rebase all the subsequent patches. When 0001 settles down some more then rebase the others. ~~ Also, please run the indentation tool over this code ASAP. ====== Kind Regards, Peter Smith. Fujitsu Australia
Attachment
pgsql-hackers by date: