Horton

Re: Minimal logical decoding on standbys

bdrouvotAWS <bdrouvot@amazon.com>

View thread
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
To: <fabriziomello@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Andres Freund <andres@anarazel.de>, tushar <tushar.ahuja@enterprisedb.com>, "[pgdg] Robert Haas" <robertmhaas@gmail.com>, Amit Khandekar <amitdkhan.pg@gmail.com>, Rahila Syed <rahila.syed@2ndquadrant.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2021-03-23T17:31:48Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Reduce the log level in 035_standby_logical_decoding.pl.

  2. 035_standby_logical_decoding: Add missing waits for replication

  3. For cascading replication, wake physical and logical walsenders separately

  4. Handle logical slot conflicts on standby

  5. Support invalidating replication slots due to horizon and wal_level

  6. Prevent use of invalidated logical slot in CreateDecodingContext()

  7. Replace replication slot's invalidated_at LSN with an enum

  8. Pass down table relation into more index relation functions

  9. Assert only valid flag bits are passed to visibilitymap_set()

  10. Remove unused _bt_delitems_delete() argument.

  11. Add xl_btree_delete optimization.

Attachments

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@gmail.com <mailto: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.

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