On Wed, Mar 25, 2026 at 2:19 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Swada-San. Here are some minor review comments for v4-0001/2 combined.
>
> ======
> src/backend/catalog/pg_publication.c
>
> is_table_publishable_in_publication:
>
> 1.
> This function logic has a format like
>
> if (cond)
> {
> ...
> return;
> }
>
> if (cond2)
> {
> ...
> return;
> }
>
> etc.
>
> There are many return points, and most of those "if" blocks cannot
> fall through (they return).
>
> I found it slightly difficult to read the code because I kept having
> to think, "OK, if we reached here, it means pubviaroot must be false,"
> or "OK, if we reached this far, then puballtables must be false, and
> pubviaroot must be false," etc.
>
I can't say exactly why, but I find it difficult to read this
function. So, I share your concerns about the code of this function.
Because of its complexity it is difficult to ascertain that the
functionality is correct or we missed something. Also, considering it
is correct today, in its current form, it may become difficult to
enhance it in future.
One more comment on latest patch:
*
+static Datum
+pg_get_publication_tables(FunctionCallInfo fcinfo, ArrayType *pubnames,
+ Oid target_relid, bool filter_by_relid,
Why do we need filter_by_relid as a separate parameter? Isn't the
valid value of target_relid the same? If so, can't we use target_relid
for the required checks?
--
With Regards,
Amit Kapila.
