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 1mE16p-0007pS-3Q for pgsql-hackers@arkaria.postgresql.org; Thu, 12 Aug 2021 03:10:19 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1mE16m-0003Fh-Bx for pgsql-hackers@arkaria.postgresql.org; Thu, 12 Aug 2021 03:10:16 +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 1mE16m-0003FZ-0r for pgsql-hackers@lists.postgresql.org; Thu, 12 Aug 2021 03:10:16 +0000 Received: from mail-yb1-xb2e.google.com ([2607:f8b0:4864:20::b2e]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1mE16j-0004vJ-GV for pgsql-hackers@postgresql.org; Thu, 12 Aug 2021 03:10:15 +0000 Received: by mail-yb1-xb2e.google.com with SMTP id z128so8778323ybc.10 for ; Wed, 11 Aug 2021 20:10:13 -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=1/1yZfBVWRgiTnGCQKWh44Vag9d4/TLueYBJhDYWU3c=; b=DEWfv8B/DvhXwX2AtVuQx1YUmL0LzICy7ZcqmUvyORYhZXGcrz97ZGQ88XJYXkYXg2 nQ5/aZt+lXcOPFlL26DE6lUMDF1tCR/USCpa9L2yUfRraKzOHismBUJKUYzOTmqlE8iU 8Kri+e6CaJf7Ow7bz5GN6G/lw01hkz8gDZlVw11FOIiRyXzGCNEuYRx4Uj6oSiNwKbJ5 xBxw2u2tR/j+qVR6MYlUpUCb7eb4+8s7oskVZUMqMRiBCZptGzO/7+HeqH8g/aRogUBg jb8yR+G+rG7BG+Z3EiT9iqQY3v83DsO1DNaADnllQ9mtHPERBRmVB6HDkjFrq+2XoDbn FYVQ== 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=1/1yZfBVWRgiTnGCQKWh44Vag9d4/TLueYBJhDYWU3c=; b=KXeCj5v5ToJVcAjwPIpZ3kjlLFLxmcap8NO0jHL5fRazQcGs/74iRkTErEn4lqFVSE dv0Qr2sV2iyYsQfeIW4eL26YSaPigkfD+O7pM8tsO95GeYy+tVaU6/UskIv1jg5Cs8iL QmvFLssm7ajLFsdYa3cx+BfN8d236igo1Jycfs3pjkQ/16asPSX9RFO5f70DQX7vQk8/ 8m5WYD5exa+C2bCrU+eLgVaFgRjFUb6DHpL6fR2NP6H7u4u0Osesf2ke/eBkjd2lCh5F meB92pyRRxPm6Gyam04ZOALeuHNDoye5jKq2m6Jy7RWxNEqOYUBgrToKmtqPU2249ZOx +Xhw== X-Gm-Message-State: AOAM5311K/TpSDCF3JAM61R7k4R2NxnsT/HYS5riSGEWx74GFJJqsepm Q4UoMUXPddEoW6a58mJCOacHgtzQzQYVpvgDPl8= X-Google-Smtp-Source: ABdhPJxvE4MM+LCGsVsZEqs8MXkPD/HJvjH0doPQCh2mLyFgGBrrxLUZBtNvsy7MeZxnDNBus7Gm21S1SlpfB/fmJwM= X-Received: by 2002:a25:6c04:: with SMTP id h4mr1599297ybc.122.1628737811684; Wed, 11 Aug 2021 20:10:11 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Rahila Syed Date: Thu, 12 Aug 2021 08:40:00 +0530 Message-ID: Subject: Re: Column Filtering in Logical Replication To: Amit Kapila Cc: Tomas Vondra , Alvaro Herrera , Peter Smith , PostgreSQL-development Content-Type: multipart/alternative; boundary="000000000000b2a1bf05c95412a4" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000b2a1bf05c95412a4 Content-Type: text/plain; charset="UTF-8" Hi Amit, Thanks for your review. > Can you please explain why you have the restriction for including > replica identity columns and do we want to put a similar restriction > for the primary key? As far as I understand, if we allow default > values on subscribers for replica identity, then probably updates, > deletes won't work as they need to use replica identity (or PK) to > search the required tuple. If so, shouldn't we add this restriction > only when a publication has been defined for one of these (Update, > Delete) actions? > Yes, like you mentioned they are needed for Updates and Deletes to work. The restriction for including replica identity columns in column filters exists because In case the replica identity column values did not change, the old row replica identity columns are not sent to the subscriber, thus we would need new replica identity columns to be sent to identify the row that is to be Updated or Deleted. I haven't tested if it would break Insert as well though. I will update the patch accordingly. > Another point is what if someone drops the column used in one of the > publications? Do we want to drop the entire relation from publication > or just remove the column filter or something else? > > Thanks for pointing this out. Currently, this is not handled in the patch. I think dropping the column from the filter would make sense on the lines of the table being dropped from publication, in case of drop table. > Do we want to consider that the columns specified in the filter must > not have NOT NULL constraint? Because, otherwise, the subscriber will > error out inserting such rows? > > I think you mean columns *not* specified in the filter must not have NOT NULL constraint on the subscriber, as this will break during insert, as it will try to insert NULL for columns not sent by the publisher. I will look into fixing this. Probably this won't be a problem in case the column is auto generated or contains a default value. > Minor comments: > ================ > pq_sendbyte(out, flags); > - > /* attribute name */ > pq_sendstring(out, NameStr(att->attname)); > > @@ -953,6 +1000,7 @@ logicalrep_write_attrs(StringInfo out, Relation rel) > > /* attribute mode */ > pq_sendint32(out, att->atttypmod); > + > } > > bms_free(idattrs); > diff --git a/src/backend/replication/logical/relation.c > b/src/backend/replication/logical/relation.c > index c37e2a7e29..d7a7b00841 100644 > --- a/src/backend/replication/logical/relation.c > +++ b/src/backend/replication/logical/relation.c > @@ -354,7 +354,6 @@ logicalrep_rel_open(LogicalRepRelId remoteid, > LOCKMODE lockmode) > > attnum = logicalrep_rel_att_by_name(remoterel, > NameStr(attr->attname)); > - > entry->attrmap->attnums[i] = attnum; > > There are quite a few places in the patch that contains spurious line > additions or removals. > > Thank you for your comments, I will fix these. Thank you, Rahila Syed --000000000000b2a1bf05c95412a4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Amit,

Thanks for your review.


Can you please explain why you have the restriction for including
replica identity columns and do we want to put a similar restriction
for the primary key? As far as I understand, if we allow default
values on subscribers for replica identity, then probably updates,
deletes won't work as they need to use replica identity (or PK) to
search the required tuple. If so, shouldn't we add this restriction
only when a publication has been defined for one of these (Update,
Delete) actions?
=C2=A0
Yes, like you mentio= ned they are needed for Updates and Deletes to work.
The restriction for= including replica identity columns in column filters exists because
<= div>In case the replica identity column values did not change, the old row = replica identity columns
are not sent to the subscriber,=C2=A0thus we w= ould need new replica identity columns
to be sent to identify=C2=A0the = row that is to be Updated or Deleted.
I haven't tested if it would = break=C2=A0Insert as well=C2=A0 though. I will update the patch accordingly= .


Another point is what if someone drops the column used in one of the
publications? Do we want to drop the entire relation from publication
or just remove the column filter or something else?


Thanks for pointing this out. Currentl= y, this is not handled in the patch.
I think dropping the column from th= e filter would make sense on the lines
of the table being dropped from p= ublication, in case of drop table.
=C2=A0
Do we want to consider that the columns specified in the filter must
not have NOT NULL constraint? Because, otherwise, the subscriber will
error out inserting such rows?

I think you mean columns *not* specified in the filte= r must not have NOT NULL constraint
on the subscriber, as this will brea= k during insert, as it will try to insert NULL for columns
not sent by t= he publisher.
I will look into fixing this. Probably this won't be = a problem in
case the column is auto generated or contains a default val= ue.
=C2=A0
Minor comments:
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=C2=A0 pq_sendbyte(out, flags);
-
=C2=A0 /* attribute name */
=C2=A0 pq_sendstring(out, NameStr(att->attname));

@@ -953,6 +1000,7 @@ logicalrep_write_attrs(StringInfo out, Relation rel)
=C2=A0 /* attribute mode */
=C2=A0 pq_sendint32(out, att->atttypmod);
+
=C2=A0 }

=C2=A0 bms_free(idattrs);
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index c37e2a7e29..d7a7b00841 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -354,7 +354,6 @@ logicalrep_rel_open(LogicalRepRelId remoteid,
LOCKMODE lockmode)

=C2=A0 attnum =3D logicalrep_rel_att_by_name(remoterel,
=C2=A0 NameStr(attr->attname));
-
=C2=A0 entry->attrmap->attnums[i] =3D attnum;

There are quite a few places in the patch that contains spurious line
additions or removals.


Thank you for your comments, I will fi= x these.=C2=A0

Thank you,
Rahila Syed
--000000000000b2a1bf05c95412a4--