Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1m5QOg-0006EP-Tc for pgsql-hackers@arkaria.postgresql.org; Mon, 19 Jul 2021 10:21:15 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1m5QOf-0003hm-UW for pgsql-hackers@arkaria.postgresql.org; Mon, 19 Jul 2021 10:21:13 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1m5QOf-0003he-LX for pgsql-hackers@lists.postgresql.org; Mon, 19 Jul 2021 10:21:13 +0000 Received: from mail-vs1-xe2a.google.com ([2607:f8b0:4864:20::e2a]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1m5QOd-0006j6-Cy for pgsql-hackers@postgresql.org; Mon, 19 Jul 2021 10:21:13 +0000 Received: by mail-vs1-xe2a.google.com with SMTP id j2so9034619vsi.6 for ; Mon, 19 Jul 2021 03:21:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=cMRkHHhbq9VmiOvWlU7qPRletppEU4vT/EHV/AzbRHk=; b=EwGVODNeY5kIg6/6Q7Aav9fLEQATtJQoHtyMQGTx0z+BDrBsu3fjte7Ar2xRRseUqN 1cTwUwT/NwevM2InKRBILbxzqjorvElgGiSzoORJGpXtWeBrfLvX98C9AK/yd3kJNBle ty8FH5qOMlkYnxpTW9vdTHLKSG4I/bCDnzQSueAWAu3f0WIZxp7sobpAB1KAI3Cppli6 LNjHfcxOK7iAK3QvA7SZyJQW5gDPJ0TATe58tuIDfY9anu+6A3x7QPBEDHAyD2Kxb4B+ 15aH2RK58xUCkcZenPSEogGjfWiT8urNBLyWU0+9EgmsjcGFzBctaUZvIvI/1/QT6VGY wuCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=cMRkHHhbq9VmiOvWlU7qPRletppEU4vT/EHV/AzbRHk=; b=Em7NVTxOVJ9BcORcCUtKh+SfZoENwBBEydntunxbGBhb0Nkp0+1ilLiaLLpYs6Ash3 PG8a2XL3Ko8IRDk/qAQTZjNjV5abZDtMgy8oFAeqZ3JRbcwQMjkQmtyq7hjrBeIQ6p8w Dvhgmrtoj5dH2c8EgNhrN0CEtpWjIck6QFDg0Dj0vGwfxVfZmV6ntRWS9s5LZ9zap8WF vovis3eeStuUq1HW0Ck6lwzh2ZRSL4Da1UDf62s6meTlKtlKTf6TtbQZQLaF9Oo53qCF DdwUG3yfmgVyN86SKRWY8QgFAtVI2D3IgrK2ehS/H0t+XCbG8lbiDbdcxHHo7UbvfRCm J4Tw== X-Gm-Message-State: AOAM533Wfz2i1YQ7Dv6dvlNFXwnjWdUxyKr6gN75+ptWc/Jm3F/0m0K9 bnjhds5+tBVumNKZsxrIaIAymAoZ57ca76rSCJQ= X-Google-Smtp-Source: ABdhPJxU/2rxI2NqMo5R6X17TVnHll831Ug0Hbi6OL/iTZKzciNhbjRUc7vcj+OdP97uO+ZaNAB3oa3t4hP5cSQU1kg= X-Received: by 2002:a67:441:: with SMTP id 62mr23727520vse.20.1626690068940; Mon, 19 Jul 2021 03:21:08 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Ibrar Ahmed Date: Mon, 19 Jul 2021 15:20:32 +0500 Message-ID: Subject: Re: Column Filtering in Logical Replication To: Rahila Syed Cc: Tomas Vondra , Peter Smith , PostgreSQL-development Content-Type: multipart/alternative; boundary="000000000000b80c6f05c7774bbc" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000b80c6f05c7774bbc Content-Type: text/plain; charset="UTF-8" On Tue, Jul 13, 2021 at 7:44 PM Rahila Syed wrote: > > Hi Tomas, > > Thank you for your comments. > > >> >> > >> > Currently, this capability is not included in the patch. If the table on >> > the subscriber >> > server has lesser attributes than that on the publisher server, it >> > throws an error at the >> > time of CREATE SUBSCRIPTION. >> > >> >> That's a bit surprising, to be honest. I do understand the patch simply >> treats the filtered columns as "unchanged" because that's the simplest >> way to filter the *data* of the columns. But if someone told me we can >> "filter columns" I'd expect this to work without the columns on the >> subscriber. >> >> OK, I will look into adding this. > > >> >> > However, need to carefully consider situations in which a server >> > subscribes to multiple >> > publications, each publishing a different subset of columns of a >> table. > > Isn't that pretty much the same situation as for multiple subscriptions >> each with a different set of I/U/D operations? IIRC we simply merge >> those, so why not to do the same thing here and merge the attributes? >> >> > Yeah, I agree with the solution to merge the attributes, similar to how > operations are merged. My concern was also from an implementation point > of view, will it be a very drastic change. I now had a look at how remote > relation > attributes are acquired for comparison with local attributes at the > subscriber. > It seems that the publisher will need to send the information about the > filtered columns > for each publication specified during CREATE SUBSCRIPTION. > This will be read at the subscriber side which in turn updates its cache > accordingly. > Currently, the subscriber expects all attributes of a published relation > to be present. > I will add code for this in the next version of the patch. > > To nitpick, I find "Bitmapset *att_list" a bit annoying, because it's > > not really a list ;-) > > > I will make this change with the next version > > > >> FWIW "make check" fails for me with this version, due to segfault in >> OpenTableLists. Apparenly there's some confusion - the code expects the >> list to contain PublicationTable nodes, and tries to extract the >> RangeVar from the elements. But the list actually contains RangeVar, so >> this crashes and burns. See the attached backtrace. >> >> > Thank you for the report, This is fixed in the attached version, now all > publication > function calls accept the PublicationTableInfo list. > > Thank you, > Rahila Syed > > > The patch does not apply, and an rebase is required Hunk #8 succeeded at 1259 (offset 99 lines). Hunk #9 succeeded at 1360 (offset 99 lines). 1 out of 9 hunks FAILED -- saving rejects to file src/backend/replication/pgoutput/pgoutput.c.rej patching file src/include/catalog/pg_publication.h Changing the status to "Waiting on Author" -- Ibrar Ahmed --000000000000b80c6f05c7774bbc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Jul 13, 2021 at 7:44 PM Rahil= a Syed <rahilasyed90@gmail.com= > wrote:
=

Hi Tomas,

Thank you for your comments.
= =C2=A0

> =C2=A0
> Currently, this capability is not included in the patch. If the table = on
> the subscriber
> server has lesser attributes than that on the publisher server, it
> throws an error at the=C2=A0
> time of CREATE SUBSCRIPTION.
>

That's a bit surprising, to be honest. I do understand the patch simply=
treats the filtered columns as "unchanged" because that's the= simplest
way to filter the *data* of the columns. But if someone told me we can
"filter columns" I'd expect this to work without the columns = on the
subscriber.

OK, I will look into adding this.=C2=A0
=C2= =A0

> However, need to carefully consider situations in which a server
> subscribes to multiple=C2=A0
> publications,=C2=A0 each publishing a different subset of columns of a= table.=C2=A0
Isn't that pretty much the same situation as for multiple subscription= s
each with a different set of I/U/D operations? IIRC we simply merge
those, so why not to do the same thing here and merge the attributes?

=C2=A0
Yeah, I agree with the solution to m= erge the attributes, similar to how=C2=A0
operations are merged. = My concern was also from an implementation point=C2=A0
of view, w= ill it be a very drastic change. I now had a look at how remote relation
attributes are acquired for comparison with local attributes at the= subscriber.=C2=A0
It seems that the publisher will need to send = the information about the filtered columns=C2=A0
for each publica= tion specified during CREATE SUBSCRIPTION.
This will be read at t= he subscriber side which=C2=A0in turn updates its=C2=A0cache accordingly.= =C2=A0
Currently, the subscriber expects all attributes of a publ= ished relation to be present.=C2=A0
I will add code for this in the next= version of the patch.

=C2=A0To nitpick, I find "Bitmapset *att_list&q= uot; a bit annoying, because it's=C2=A0=C2=A0
not really a list ;-)
=C2=A0
I will make this change with the next version
= =C2=A0
=C2=A0
=C2=A0FWIW "make check" fails for me with this ve= rsion, due to segfault in
OpenTableLists. Apparenly there's some c= onfusion - the code expects the
list to contain PublicationTable nodes, = and tries to extract the
RangeVar from the elements. But the list actual= ly contains RangeVar, so
this crashes and burns. See the attached backtr= ace.

=C2=A0
Thank you for the= report, This is=C2=A0fixed in the attached version, now all publication
function calls=C2=A0accept the PublicationTableInfo list.

Th= ank you,
Rahila Syed

=C2=A0 =C2=A0=C2=A0=

The patch does not apply, and an rebase i= s required
Hunk #8=
 succeeded at 1259 (offset 99 lines).
Hunk #9 succeeded at 1360 (offset 99 lines).
1 out of 9 hunks FAILED -- saving rejects to file src/backend/replication/p=
goutput/pgoutput.c.rej
patching file src/include/catalog/pg_publication.h

Changing the statu= s to "Waiting on Author"

--
Ibrar Ahmed
=
--000000000000b80c6f05c7774bbc--