RE: Data is copied twice when specifying both child and parent table in publication

On Wed, Oct 5, 2022 at 11:08 AM Peter Smith <smithpb2250@gmail.com> wrote:
> Hi Wang-san. Here are my review comments for HEAD_v12-0001 patch.

Thanks for your comments.

> ======
> 
> 1. Missing documentation.
> 
> In [1] you wrote:
> > I think the behaviour of multiple publications with parameter
> publish_via_partition_root could be added to the pg-doc later in a separate
> patch.
> 
> ~
> 
> That doesn't seem right to me. IMO the related documentation updates
> cannot really be separated from this patch. Otherwise, what's the
> alternative? Push this change, and then (while waiting for the
> documentation patch) users will just have to use trial and error to
> guess how it works...?

I tried to add related documentation in a separate patch (HEAD_v13-0002*).

> ------
> 
> 2. src/backend/catalog/pg_publication.c
> 
> + typedef struct
> + {
> + Oid relid; /* OID of published table */
> + Oid pubid; /* OID of publication that publishes this
> + * table. */
> + } published_rel;
> 
> 2a.
> I think that should be added to typedefs.list

Added.

> ~
> 
> 2b.
> Maybe this also needs some comment to clarify that there will be
> *multiple* of these structures in scenarios where the same table is
> published by different publications in the array passed.

Added the comments.

> ------
> 
> 3. QUESTION - pg_get_publication_tables / fetch_table_list
> 
> When the same table is published by different publications (but there
> are other differences like row-filters/column-lists in each
> publication) the result tuple of this function does not include the
> pubid. Maybe the SQL of pg_publication_tables/fetch_table_list() is OK
> as-is but how does it manage to associate each table with the correct
> tuple?
> 
> I know it apparently all seems to work but I’m not how does that
> happen? Can you explain why a puboid is not needed for the result
> tuple of this function?

Sorry, I am not sure I understand your question.
I try to answer your question by explaining the two functions you mentioned:

First, the function pg_get_publication_tables gets the list (see table_infos)
that included published table and the corresponding publication. Then based
on this list, the function pg_get_publication_tables returns information
(scheme, relname, row filter and column list) about the published tables in the
publications list. It just doesn't return pubid.

Then, the SQL in the function fetch_table_list will get the columns in the
column list from pg_attribute. (This is to return all columns when the column
list is not specified)

> ~~
> 
> test_pub=# create table t1(a int, b int, c int);
> CREATE TABLE
> test_pub=# create publication pub1 for table t1(a) where (a > 99);
> CREATE PUBLICATION
> test_pub=# create publication pub2 for table t1(a,b) where (b < 33);
> CREATE PUBLICATION
> 
> Following seems OK when I swap orders of publication names...
> 
> test_pub=# SELECT gpt.relid, gpt.attrs, pg_get_expr(gpt.qual,
> gpt.relid) AS rowfilter from pg_get_publication_tables(VARIADIC
> ARRAY['pub2','pub1']) gpt(relid, attrs, qual);
>  relid | attrs | rowfilter
> -------+-------+-----------
>  16385 | 1 2   | (b < 33)
>  16385 | 1     | (a > 99)
> (2 rows)
> 
> test_pub=# SELECT gpt.relid, gpt.attrs, pg_get_expr(gpt.qual,
> gpt.relid) AS rowfilter from pg_get_publication_tables(VARIADIC
> ARRAY['pub1','pub2']) gpt(relid, attrs, qual);
>  relid | attrs | rowfilter
> -------+-------+-----------
>  16385 | 1     | (a > 99)
>  16385 | 1 2   | (b < 33)
> (2 rows)
> 
> But what about this (this is similar to the SQL fragment from
> fetch_table_list); I swapped the pub names but the results are the
> same...
> 
> test_pub=# SELECT pg_get_publication_tables(VARIADIC
> array_agg(p.pubname)) from pg_publication p where pubname
> IN('pub2','pub1');
> 
>                      pg_get_publication_tables
> 
> -------------------------------------------------------------------------------------------------
> ---------------------------------------------------------------------
> -------------------------------------------------------------------------------------------------
> ---------------------------------------------------------------------
> -------------------------------------------------------------------
>  (16385,1,"{OPEXPR :opno 521 :opfuncid 147 :opresulttype 16 :opretset
> false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 1
> :vartype 23 :vartypmod -1 :var
> collid 0 :varlevelsup 0 :varnosyn 1 :varattnosyn 1 :location 47}
> {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4
> :constbyval true :constisnull false :
> location 51 :constvalue 4 [ 99 0 0 0 0 0 0 0 ]}) :location 49}")
>  (16385,"1 2","{OPEXPR :opno 97 :opfuncid 66 :opresulttype 16
> :opretset false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1
> :varattno 2 :vartype 23 :vartypmod -1 :v
> arcollid 0 :varlevelsup 0 :varnosyn 1 :varattnosyn 2 :location 49}
> {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4
> :constbyval true :constisnull false
>  :location 53 :constvalue 4 [ 33 0 0 0 0 0 0 0 ]}) :location 51}")
> (2 rows)
> 
> test_pub=# SELECT pg_get_publication_tables(VARIADIC
> array_agg(p.pubname)) from pg_publication p where pubname
> IN('pub1','pub2');
> 
>                      pg_get_publication_tables
> 
> -------------------------------------------------------------------------------------------------
> ---------------------------------------------------------------------
> -------------------------------------------------------------------------------------------------
> ---------------------------------------------------------------------
> -------------------------------------------------------------------
>  (16385,1,"{OPEXPR :opno 521 :opfuncid 147 :opresulttype 16 :opretset
> false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 1
> :vartype 23 :vartypmod -1 :var
> collid 0 :varlevelsup 0 :varnosyn 1 :varattnosyn 1 :location 47}
> {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4
> :constbyval true :constisnull false :
> location 51 :constvalue 4 [ 99 0 0 0 0 0 0 0 ]}) :location 49}")
>  (16385,"1 2","{OPEXPR :opno 97 :opfuncid 66 :opresulttype 16
> :opretset false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1
> :varattno 2 :vartype 23 :vartypmod -1 :v
> arcollid 0 :varlevelsup 0 :varnosyn 1 :varattnosyn 2 :location 49}
> {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4
> :constbyval true :constisnull false
>  :location 53 :constvalue 4 [ 33 0 0 0 0 0 0 0 ]}) :location 51}")
> (2 rows)

I think this is because the usage of SELECT statement. The order seems depend
on pg_publication. Such as:

postgres=# SELECT array_agg(p.pubname) FROM pg_publication p WHERE pubname IN ('pub1','pub2');
  array_agg
-------------
 {pub1,pub2}
(1 row)

postgres=# SELECT array_agg(p.pubname) FROM pg_publication p WHERE pubname IN ('pub2','pub1');
  array_agg
-------------
 {pub1,pub2}
(1 row)

Attach the new patch set.

Regards,
Wang wei