public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
To: Amit Kapila <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Rahila Syed <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Column Filtering in Logical Replication
Date: Sat, 26 Mar 2022 01:29:35 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA4eK1LY_JGL7LvdT64ujEiEAVaADuhdej1QNnwxvO_-KPzeEg@mail.gmail.com>
References: <[email protected]>
<CAA4eK1J9UDnM-xN7viu4eO_DDL_PqMG9F6ZsskqEU7snujXi+A@mail.gmail.com>
<[email protected]>
<CAA4eK1KUpOnFam+2bBuGGX-_pZbmO_ShJV6G=GT-RyM7gQSvWg@mail.gmail.com>
<[email protected]>
<OS0PR01MB57164514E1DE1E579B0B6C46940F9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<CAA4eK1KvFAygPbFu=cUvnqhmTLPpjzqp8p4UNSsLkiZmvFp8oQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAA4eK1LpBFU49Ohbnk=dv_v9YP+Kqh1+Sf8i++_s-QhD1Gy4Qw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAA4eK1LY_JGL7LvdT64ujEiEAVaADuhdej1QNnwxvO_-KPzeEg@mail.gmail.com>
On 3/21/22 15:12, Amit Kapila wrote:
> On Sat, Mar 19, 2022 at 11:11 PM Tomas Vondra
> <[email protected]> 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
view thread (185+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Column Filtering in Logical Replication
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox