Message-ID: <05353cce-2da8-74cc-5419-d0a398212f12@enterprisedb.com>
Date: Sat, 26 Mar 2022 01:29:35 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: Column Filtering in Logical Replication
Content-Language: en-US
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Justin Pryzby <pryzby@telsasoft.com>, Rahila Syed <rahilasyed90@gmail.com>,
 Peter Smith <smithpb2250@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
 "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
References: <202201120041.p24wvsfcsope@alvherre.pgsql>
 <CAA4eK1J9UDnM-xN7viu4eO_DDL_PqMG9F6ZsskqEU7snujXi+A@mail.gmail.com>
 <2b8471f9-3849-c91f-ef82-9181d4bca0a7@enterprisedb.com>
 <CAA4eK1KUpOnFam+2bBuGGX-_pZbmO_ShJV6G=GT-RyM7gQSvWg@mail.gmail.com>
 <16e83ce3-8415-b732-37b2-c3329a2c13a1@enterprisedb.com>
 <OS0PR01MB57164514E1DE1E579B0B6C46940F9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1KvFAygPbFu=cUvnqhmTLPpjzqp8p4UNSsLkiZmvFp8oQ@mail.gmail.com>
 <bc6918cc-2025-6c91-2ca4-c237af67e97d@enterprisedb.com>
 <f217341d-6841-390d-3dd2-89f9d1de17fe@enterprisedb.com>
 <f90d2b03-4462-ce95-a524-d91464e797c8@enterprisedb.com>
 <CAA4eK1LpBFU49Ohbnk=dv_v9YP+Kqh1+Sf8i++_s-QhD1Gy4Qw@mail.gmail.com>
 <3162f7f0-86ac-e3a9-dc55-0a660cb9bab1@enterprisedb.com>
 <f987cbd5-89b1-9afb-4c6d-824eae0eec40@enterprisedb.com>
 <9f551740-1395-f73d-a792-9d487f8074db@enterprisedb.com>
 <566a06cd-fcb5-3e78-b2f0-8d21d92e5b87@enterprisedb.com>
 <CAA4eK1LY_JGL7LvdT64ujEiEAVaADuhdej1QNnwxvO_-KPzeEg@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CAA4eK1LY_JGL7LvdT64ujEiEAVaADuhdej1QNnwxvO_-KPzeEg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/05353cce-2da8-74cc-5419-d0a398212f12%40enterprisedb.com>
Precedence: bulk

On 3/21/22 15:12, Amit Kapila wrote:
> On Sat, Mar 19, 2022 at 11:11 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> On 3/19/22 18:11, Tomas Vondra wrote:
>>> Fix a compiler warning reported by cfbot.
>>
>> Apologies, I failed to actually commit the fix. So here we go again.
>>
> 
> Few comments:
> ===============
> 1.
> +/*
> + * Gets a list of OIDs of all partial-column publications of the given
> + * relation, that is, those that specify a column list.
> + */
> +List *
> +GetRelationColumnPartialPublications(Oid relid)
> {
> ...
> }
> 
> ...
> +/*
> + * For a relation in a publication that is known to have a non-null column
> + * list, return the list of attribute numbers that are in it.
> + */
> +List *
> +GetRelationColumnListInPublication(Oid relid, Oid pubid)
> {
> ...
> }
> 
> Both these functions are not required now. So, we can remove them.
> 

Good catch, removed.

> 2.
> @@ -464,11 +478,11 @@ logicalrep_write_update(StringInfo out,
> TransactionId xid, Relation rel,
>   pq_sendbyte(out, 'O'); /* old tuple follows */
>   else
>   pq_sendbyte(out, 'K'); /* old key follows */
> - logicalrep_write_tuple(out, rel, oldslot, binary);
> + logicalrep_write_tuple(out, rel, oldslot, binary, columns);
>   }
> 
> As mentioned previously, here, we should pass NULL similar to
> logicalrep_write_delete as we don't need to use column list for old
> tuples.
> 

Fixed.

> 3.
> + * XXX The name is a bit misleading, because we don't really transform
> + * anything here - we merely check the column list is compatible with the
> + * definition of the publication (with publish_via_partition_root=false)
> + * we only allow column lists on the leaf relations. So maybe rename it?
> + */
> +static void
> +TransformPubColumnList(List *tables, const char *queryString,
> +    bool pubviaroot)
> 
> The second parameter is not used in this function. As noted in the
> comments, I also think it is better to rename this. How about
> ValidatePubColumnList?
> 
> 4.
> @@ -821,6 +942,9 @@ fetch_remote_table_info(char *nspname, char *relname,
>   *
>   * 3) one of the subscribed publications is declared as ALL TABLES IN
>   * SCHEMA that includes this relation
> + *
> + * XXX Does this actually handle puballtables and schema publications
> + * correctly?
>   */
>   if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
> 
> Why is this comment added in the row filter code? Now, both row filter
> and column list are fetched in the same way, so not sure what exactly
> this comment is referring to.
> 

I added that comment as a note to myself while learning about how the
code works, forgot to remove that.

> 5.
> +/* qsort comparator for attnums */
> +static int
> +compare_int16(const void *a, const void *b)
> +{
> + int av = *(const int16 *) a;
> + int bv = *(const int16 *) b;
> +
> + /* this can't overflow if int is wider than int16 */
> + return (av - bv);
> +}
> 
> The exact same code exists in statscmds.c. Do we need a second copy of the same?
> 

Yeah, I thought about moving it to some common header, but I think it's
not really worth it at this point.

> 6.
>  static void pgoutput_row_filter_init(PGOutputData *data,
>   List *publications,
>   RelationSyncEntry *entry);
> +
>  static bool pgoutput_row_filter_exec_expr(ExprState *state,
> 
> Spurious line addition.
> 

Fixed.

> 7. The tests in 030_column_list.pl take a long time as compared to all
> other similar individual tests in the subscription folder. I haven't
> checked whether there is any need to reduce some tests but it seems
> worth checking.
> 

On my machine, 'make check' in src/test/subscription takes ~150 seconds
(with asserts and -O0), and the new script takes ~14 seconds, while most
other tests have 3-6 seconds.

AFAICS that's simply due to the number of tests in the script, and I
don't think there are any unnecessary ones. I was actually adding them
in response to issues reported during development, or to test various
important cases. So I don't think we can remove some of them easily :-(

And it's not like the tests are using massive amounts of data either.

We could split the test, but that obviously won't reduce the duration,
of course.

So I decided to keep the test as is, for now, and maybe we can try
reducing the test after a couple buildfarm runs.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company