From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
Cc: | Shubham Khanna <khannashubham1197(at)gmail(dot)com>, Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, euler(at)eulerto(dot)com |
Subject: | Re: Pgoutput not capturing the generated columns |
Date: | 2024-07-05 08:17:22 |
Message-ID: | CAHut+PuWiDazDJVo1y1B2yr6cnLD-yMFQ-LRB+X8y60AX-bBhA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Here are my review comments for v14-0002.
======
src/backend/replication/logical/tablesync.c
make_copy_attnamelist:
nitpick - remove excessive parentheses in palloc0 call.
nitpick - Code is fine AFAICT except it's not immediately obvious
localgenlist is indexed by the *remote* attribute number. So I renamed
'attrnum' variable in my nitpicks diff. OTOH, if you think no change
is necessary, that is OK to (in that case maybe add a comment).
~~~
1. fetch_remote_table_info
+ if ((server_version >= 120000 && server_version <= 160000) ||
+ !MySubscription->includegencols)
+ appendStringInfo(&cmd, " AND a.attgenerated = ''");
Should this say < 180000 instead of <= 160000?
~~~
copy_table:
nitpick - uppercase in comment
nitpick - missing space after "if"
~~~
2. copy_table
+ attnamelist = make_copy_attnamelist(relmapentry, remotegenlist);
+
/* Start copy on the publisher. */
initStringInfo(&cmd);
- /* Regular table with no row filter */
- if (lrel.relkind == RELKIND_RELATION && qual == NIL)
+ /* check if remote column list has generated columns */
+ if(MySubscription->includegencols)
+ {
+ for (int i = 0; i < relmapentry->remoterel.natts; i++)
+ {
+ if(remotegenlist[i])
+ {
+ remote_has_gencol = true;
+ break;
+ }
+ }
+ }
+
There is some subtle logic going on here:
For example, the comment here says "Check if the remote column list
has generated columns", and it then proceeds to iterate the remote
attributes checking the remotegenlist[i]. But the remotegenlist[] was
returned from a prior call to make_copy_attnamelist() and according to
the make_copy_attnamelist logic, it is NOT returning all remote
generated-cols in that list. Specifically, it is stripping some of
them -- "Do not include generated columns of the subscription table in
the [remotegenlist] column list.".
So, actually this loop seems to be only finding cases (setting
remote_has_gen = true) where the remote column is generated but the
match local column is *not* generated. Maybe this was the intended
logic all along but then certainly the comment should be improved to
describe it better.
~~~
3.
+ /*
+ * Regular table with no row filter and 'include_generated_columns'
+ * specified as 'false' during creation of subscription.
+ */
+ if (lrel.relkind == RELKIND_RELATION && qual == NIL && !remote_has_gencol)
nitpick - This comment also needs improving. For example, just because
remote_has_gencol is false, it does not follow that
'include_generated_columns' was specified as 'false' -- maybe the
parameter was 'true' but the table just had no generated columns
anyway... I've modified the comment already in my nitpicks diff, but
probably you can improve on that.
~
nitpick - "else" comment is modified slightly too. Please see the nitpicks diff.
~
4.
In hindsight, I felt your variable 'remote_has_gencol' was not
well-named because it is not for saying the remote table has a
generated column -- it is saying the remote table has a generated
column **that we have to copy**. So, rather it should be named
something like 'gencol_copy_needed' (but I didn't change this name in
the nitpick diffs...)
======
src/test/subscription/t/004_sync.pl
nitpick - changes to comment style to make the test case separations
much more obvious
nitpick - minor comment wording tweaks
5.
Here, you are confirming we get an ERROR when replicating from a
non-generated column to a generated column. But I think your patch
also added exactly that same test scenario in the 011_generated (as
the sub5 test). So, maybe this one here should be removed?
======
src/test/subscription/t/011_generated.pl
nitpick - comment wrapping at 80 chars
nitpick - add/remove blank lines for readability
nitpick - typo /subsriber/subscriber/
nitpick - prior to the ALTER test, tab6 is unsubscribed. So add
another test to verify its initial data
nitpick - sometimes the msg 'add a new table to existing publication'
is misplaced
nitpick - the tests for tab6 and tab5 were in opposite to the expected
order, so swapped them.
======
99.
Please see also the attached diff which implements all the nitpicks
described in this post.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachment | Content-Type | Size |
---|---|---|
PS_NITPICKS_20240705_GENCOLS_V140002.txt | text/plain | 11.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Daniel Gustafsson | 2024-07-05 08:26:13 | Re: speed up pg_upgrade with large number of tables |
Previous Message | David Rowley | 2024-07-05 08:06:27 | Re: Parent/child context relation in pg_get_backend_memory_contexts() |