From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Shubham Khanna <khannashubham1197(at)gmail(dot)com> |
Cc: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "euler(at)eulerto(dot)com" <euler(at)eulerto(dot)com> |
Subject: | Re: Pgoutput not capturing the generated columns |
Date: | 2024-07-15 02:38:20 |
Message-ID: | CAHut+PtqH6kA5ifH8zcLLatknY=hnZWr6vXJ8x7rzFm3HFLkMA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi, here are some review comments about patch v17-0003
======
1.
Missing a docs change?
Previously, (v16-0002) the patch included a change to
doc/src/sgml/protocol.sgml like below to say STORED generated instead
of just generated.
<para>
- Boolean option to enable generated columns. This option controls
- whether generated columns should be included in the string
- representation of tuples during logical decoding in PostgreSQL.
+ Boolean option to enable <literal>STORED</literal> generated columns.
+ This option controls whether <literal>STORED</literal>
generated columns
+ should be included in the string representation of tuples
during logical
+ decoding in PostgreSQL.
</para>
Why is that v16 change no longer present in patch v17-0003?
======
src/backend/catalog/pg_publication.c
2.
Previously, (v16-0003) this patch included a change to clarify the
kind of generated cols that are allowed in a column list.
* Translate a list of column names to an array of attribute numbers
* and a Bitmapset with them; verify that each attribute is appropriate
- * to have in a publication column list (no system or generated attributes,
- * no duplicates). Additional checks with replica identity are done later;
- * see pub_collist_contains_invalid_column.
+ * to have in a publication column list (no system or virtual generated
+ * attributes, no duplicates). Additional checks with replica identity
+ * are done later; see pub_collist_contains_invalid_column.
Why is that v16 change no longer present in patch v17-0003?
======
src/backend/replication/logical/tablesync.c
3. make_copy_attnamelist
- if (!attr->attgenerated)
+ if (attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
continue;
IIUC this logic is checking to make sure the subscriber-side table
column was not a generated column (because we don't replicate on top
of generated columns). So, does the distinction of STORED/VIRTUAL
really matter here?
~~~
fetch_remote_table_info:
nitpick - Should not change any spaces unrelated to the patch
======
send_relation_and_attrs:
- if (att->attgenerated && !include_generated_columns)
+ if (att->attgenerated && (att->attgenerated !=
ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
continue;
nitpick - It seems over-complicated. Conditions can be split so the
code fragment looks the same as in other places in this patch.
======
99.
Please see the attached diffs patch that implements any nitpicks
mentioned above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachment | Content-Type | Size |
---|---|---|
PS_NITPICKS_20240715_GENCOLS_V170003.txt | text/plain | 1.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Richard Guo | 2024-07-15 02:45:16 | Re: Wrong results with grouping sets |
Previous Message | Richard Guo | 2024-07-15 01:36:37 | Re: Check lateral references within PHVs for memoize cache keys |