Re: Minimal logical decoding on standbys

Hi,

On 3/23/21 3:29 PM, Fabrízio de Royes Mello wrote:
>
> On Tue, Mar 23, 2021 at 10:18 AM Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com <mailto:fabriziomello(at)gmail(dot)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.

Thanks for the review!

> Some minor comments:
>
> +    <para>
> +     A logical replication slot can also be created on a hot standby.
> To prevent
> +     <command>VACUUM</command> from removing required rows from the
> system
> +     catalogs, <varname>hot_standby_feedback</varname> should be set
> on the
> +     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'.
> +    </para>
>
> Remove extra space before "Existing logical slots..."

done in v13 attached.

>
> +            pg_stat_get_db_conflict_logicalslot(D.oid) AS
> confl_logicalslot,
>
> Move it to the end of pg_stat_database_conflicts columns

done in v13 attached.

>
>
> +            * is being reduced.  Hence this extra check.
>
> Remove extra space before "Hence this..."

done in v13 attached.
>
>
> +       /* Send the other backend, a conflict recovery signal */
> +
> +       SetInvalidVirtualTransactionId(vxid);
>
> Remove extra empty line

done in v13 attached.
>
>
> +               if (restart_lsn % XLOG_BLCKSZ != 0)
> +                   elog(ERROR, "invalid replay pointer");
>
> Add an empty line after this "IF" for code readability

done in v13 attached.

>
>
> +void
> +ResolveRecoveryConflictWithLogicalSlots(Oid dboid, TransactionId xid,
> +                                       char *conflict_reason)
> +{
> +   int         i;
> +   bool        found_conflict = false;
> +
> +   if (max_replication_slots <= 0)
> +       return;
>
> What about adding an "Assert(max_replication_slots >= 0);" before the
> replication slots check?

Makes sense, in v13 attached: Assert added and then also changed the
following if accordingly to "== 0".

>
> 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
> <http://022_standby_logical_decoding_xmins.pl> ->
> src/test/recovery/t/024_standby_logical_decoding_xmins.pl
> <http://024_standby_logical_decoding_xmins.pl>
> src/test/recovery/t/023_standby_logical_decoding_conflicts.pl
> <http://023_standby_logical_decoding_conflicts.pl>
> -> src/test/recovery/t/025_standby_logical_decoding_conflicts.pl
> <http://025_standby_logical_decoding_conflicts.pl>

done in v13 attached.

Bertrand