From: | vignesh C <vignesh21(at)gmail(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Shubham Khanna <khannashubham1197(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(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-10-29 09:48:16 |
Message-ID: | CALDaNm2ozWGKD93dMa6eJpDJugJ0by1S35+w5VRWsKpiVakxhQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, 29 Oct 2024 at 11:30, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are my review comments for patch v44-0002.
>
> ======
> Commit message.
>
> 1.
> The commit message is missing.
This patch is now merged, so no change required.
> ======
> src/backend/replication/logical/tablesync.c
>
> fetch_remote_table_info:
>
> 2.
> +fetch_remote_table_info(char *nspname, char *relname, LogicalRepRelation *lrel,
> + List **qual, bool *remotegencolpresent)
>
> The name 'remotegencolpresent' sounds like it means a generated col is
> present in the remote table, but don't we only care when it is being
> published? So, would a better parameter name be more like
> 'remote_gencol_published'?
I have changed it to gencol_published based on Amit's suggestion at [1].
> ~~~
>
> 3.
> Would it be better to introduce a new human-readable variable like:
> bool check_for_published_gencols = (server_version >= 180000);
>
> because then you could use that instead of having the 180000 check in
> multiple places.
I felt this is not required, so not making any change for this.
> ~~~
>
> 4.
> - lengthof(attrRow), attrRow);
> + server_version >= 180000 ? lengthof(attrRow) : lengthof(attrRow) -
> 1, attrRow);
>
> If you wish, that length calculation could be written more concisely like:
> lengthof(attrow) - (server_version >= 180000 ? 0 : 1)
I felt the current one is better, also Amit feels the same way as in
[1]. Not making any change for this.
> ~~~
>
> 5.
> + if (server_version >= 180000)
> + *remotegencolpresent |= DatumGetBool(slot_getattr(slot, 5, &isnull));
> +
>
> Should this also say Assert(!isnull)?
Added an assert
> ======
> src/test/subscription/t/031_column_list.pl
>
> 6.
> + qq(0|1),
> + 'replication with generated columns in column list');
>
> Perhaps this message should be worded slightly differently, to
> distinguish it from the "normal" replication message.
>
> /replication with generated columns in column list/initial replication
> with generated columns in column list/
Modified
The v45 version patch attached at [2] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAA4eK1Lpzy3eqd2AOM%2BTXp80SFL1cCfX3cf9thjL-hJxn%2BAYGA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CALDaNm1oc-%2Buav380Z1k6gCZY5GJn5ZYKRexwM%2BqqGiRinUS-Q%40mail.gmail.com
Regards,
Vignesh
From | Date | Subject | |
---|---|---|---|
Next Message | Daniel Gustafsson | 2024-10-29 09:57:28 | Re: Replace current implementations in crypt() and gen_salt() to OpenSSL |
Previous Message | David Rowley | 2024-10-29 09:47:22 | Re: Add ExprState hashing for GROUP BY and hashed SubPlans |