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 1lOi2h-0001VW-MR for pgsql-hackers@arkaria.postgresql.org; Tue, 23 Mar 2021 14:29:59 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lOi2g-0002VN-CD for pgsql-hackers@arkaria.postgresql.org; Tue, 23 Mar 2021 14:29:58 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lOi2f-0002VF-VJ for pgsql-hackers@lists.postgresql.org; Tue, 23 Mar 2021 14:29:58 +0000 Received: from mail-io1-xd29.google.com ([2607:f8b0:4864:20::d29]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lOi2c-0003GI-56 for pgsql-hackers@postgresql.org; Tue, 23 Mar 2021 14:29:56 +0000 Received: by mail-io1-xd29.google.com with SMTP id k25so5755453iob.6 for ; Tue, 23 Mar 2021 07:29:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:reply-to:from:date:message-id :subject:to:cc; bh=W8EsExyBAAm52qpwdv3pruEi7O647Jg7+bGRRGf5qBs=; b=TNz6tJg4zE6jY3HzQRsriZAzloQq6Rm2xOgbIDlyXkeFclST6CdFJBlfF8bgGO3C7S tA++DDDngtgXzEIUe/RT7rhfsRzCNIBIOt4zA76mhkx8/b1ZOBCcCzB2tEjoj6b8wXiP cE9amse6m9aESS/CeDKCa471lsKojeSbW+H7+hNoKDU/Rq9XiyDtnse/7RSyb7YvsT9U GTH9TWEHrrdYVnplGH9Df3nCaXsLha/yNNf/iD9CroT4U8uLmlK+X2iwX4Tb4c4t26IW x5OxR3FORhCh88jTgIIAYb24pQEhqRsxuiMYGXjsMtLh9X9R2KWzkgkkBfVwqVZrbD4z KF0w== 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:reply-to :from:date:message-id:subject:to:cc; bh=W8EsExyBAAm52qpwdv3pruEi7O647Jg7+bGRRGf5qBs=; b=P09FLBxydMflN/6JlxbIiizXiPvAj6AH0JpQr+0D6+O0aN2hnQwBbFyN0PJaaBfyrv wnRuhx25/hdh0cyIdSZF57ZPTyF4uNiHtJOnY6g91KfavtnX4QUu/oZL7MPb/4idzLDL ot3gPIPOVupoGEvOmWB68kOxHgS89+Hg3/wBpE3f43Ji4sFR+tdsozO5xxJMl4qW6rKA nQx39serytdSYepln4b+yAwtkWC6ZjuMVrYRPque+XBIx0Nja6FHJyLbW86KV47xeIqi abmG+Wi6aOPRMWdPE1EcIsy9B1qbE18C/91Ia/Bvd2P7Xs8mB0BWcWnPKE/6iLPLEsVH ACuA== X-Gm-Message-State: AOAM533ge26BFsnTXt0H1dZehcmonIq5A2ej4qkeEant5MUdDDDKWFim ooozpyL4BJNCc4a1cr3lmMOZhxPFCzZQBPTq8pk= X-Google-Smtp-Source: ABdhPJyeHnaNXRcLrBmV8YAOceSv+vFZUtSVA8AaFAmCXVDSLcO1w5s3vYS5XHS1HjiKkqk3/gv+E6MIxtLdt5KDyW8= X-Received: by 2002:a02:714f:: with SMTP id n15mr4832409jaf.6.1616509792957; Tue, 23 Mar 2021 07:29:52 -0700 (PDT) MIME-Version: 1.0 References: <20200318184249.GA29007@alvherre.pgsql> <20200318195038.GA7123@alvherre.pgsql> <1ab609c9-2c16-26ff-5fbe-c3195e541f9c@amazon.com> <6e1dda09-0c4f-b196-a589-fb741f11686f@amazon.com> In-Reply-To: Reply-To: fabriziomello@gmail.com From: =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= Date: Tue, 23 Mar 2021 11:29:41 -0300 Message-ID: Subject: Re: Minimal logical decoding on standbys To: "Drouvot, Bertrand" Cc: Alvaro Herrera , Andres Freund , tushar , "[pgdg] Robert Haas" , Amit Khandekar , Rahila Syed , pgsql-hackers Content-Type: multipart/alternative; boundary="000000000000fc384d05be35032a" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000fc384d05be35032a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Mar 23, 2021 at 10:18 AM Fabr=C3=ADzio de Royes Mello < fabriziomello@gmail.com> wrote: > > LGTM too... Reviewing new changes now to move it forward and make this patch set ready for commiter review. > According to the feature LGTM and all tests passed. Documentation is also OK. Some minor comments: + + A logical replication slot can also be created on a hot standby. To prevent + VACUUM from removing required rows from the system + catalogs, hot_standby_feedback should be set on th= e + standby. In spite of that, if any required rows get removed, the slot gets + dropped. Existing logical slots on standby also get dropped if wal_level + on primary is reduced to less than 'logical'. + Remove extra space before "Existing logical slots..." + pg_stat_get_db_conflict_logicalslot(D.oid) AS confl_logicalslot, Move it to the end of pg_stat_database_conflicts columns + * is being reduced. Hence this extra check. Remove extra space before "Hence this..." + /* Send the other backend, a conflict recovery signal */ + + SetInvalidVirtualTransactionId(vxid); Remove extra empty line + if (restart_lsn % XLOG_BLCKSZ !=3D 0) + elog(ERROR, "invalid replay pointer"); Add an empty line after this "IF" for code readability +void +ResolveRecoveryConflictWithLogicalSlots(Oid dboid, TransactionId xid, + char *conflict_reason) +{ + int i; + bool found_conflict =3D false; + + if (max_replication_slots <=3D 0) + return; What about adding an "Assert(max_replication_slots >=3D 0);" before the replication slots check? One last thing is about the name of TAP tests, we should rename them because there are other TAP tests starting with 022_ and 023_. It should be renamed to: src/test/recovery/t/022_standby_logical_decoding_xmins.pl -> src/test/recovery/t/024_standby_logical_decoding_xmins.pl src/test/recovery/t/023_standby_logical_decoding_conflicts.pl -> src/test/recovery/t/025_standby_logical_decoding_conflicts.pl Regards, --=20 Fabr=C3=ADzio de Royes Mello PostgreSQL Developer at OnGres Inc. - https://ongres.com --000000000000fc384d05be35032a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Mar 23, 2021 at 10:18 AM Fabr=C3=ADzio de = Royes Mello <fabriziomello@gm= ail.com> wrote:
>
> LGTM too... Reviewing new changes no= w to move it forward and make this patch set ready for commiter review.
= >

According to the feature LGTM and= all tests passed. Documentation is also OK. Some minor comments:

+ =C2=A0 =C2=A0<para>
+ =C2=A0 =C2=A0 A logical re= plication slot can also be created on a hot standby. To prevent
+ =C2=A0= =C2=A0 <command>VACUUM</command> from removing required rows f= rom the system
+ =C2=A0 =C2=A0 catalogs, <varname>hot_standby_feed= back</varname> should be set on the
+ =C2=A0 =C2=A0 standby. In sp= ite of that, if any required rows get removed, the slot gets
+ =C2=A0 = =C2=A0 dropped.=C2=A0 Existing logical slots on standby also get dropped if= wal_level
+ =C2=A0 =C2=A0 on primary is reduced to less than 'logic= al'.
+ =C2=A0 =C2=A0</para>

Remov= e extra space before "Existing logical slots..."
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pg_stat_get_db_conf= lict_logicalslot(D.oid) AS confl_logicalslot,

= Move it to the end of pg_stat_database_conflicts columns


+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* is being= reduced.=C2=A0 Hence this extra check.

Remove= extra space before "Hence this..."

=
+ =C2=A0 =C2=A0 =C2=A0 /* Send the other backend, a conflict= recovery signal */
+
+ =C2=A0 =C2=A0 =C2=A0 SetInvalidVirtualTransac= tionId(vxid);

Remove extra empty line


+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 if (restart_lsn % XLOG_BLCKSZ !=3D 0)
+ =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 elog(ERROR, "invalid replay poi= nter");

Add an empty line after this &quo= t;IF" for code readability=C2=A0


+void
+ResolveRecoveryConflictWithLogicalSlots(Oid dboid, Transaction= Id xid,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 char= *conflict_reason)
+{
+ =C2=A0 int =C2=A0 =C2=A0 =C2=A0 =C2=A0 i;
= + =C2=A0 bool =C2=A0 =C2=A0 =C2=A0 =C2=A0found_conflict =3D false;
+
= + =C2=A0 if (max_replication_slots <=3D 0)
+ =C2=A0 =C2=A0 =C2=A0 ret= urn;

What about adding an "Assert(max_rep= lication_slots >=3D 0);" before the replication slots check?
<= div>
One last thing is about the name of TAP tests, we should= rename them because there are other TAP tests starting with 022_ and 023_.= It should be renamed to:


--
=C2=A0=C2=A0 Fab= r=C3=ADzio de Royes Mello
=C2=A0 =C2=A0PostgreSQL Developer at OnGres In= c. - https://ongres.com
--000000000000fc384d05be35032a--