From: | "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com> |
---|---|
To: | <fabriziomello(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, tushar <tushar(dot)ahuja(at)enterprisedb(dot)com>, "[pgdg] Robert Haas" <robertmhaas(at)gmail(dot)com>, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>, Rahila Syed <rahila(dot)syed(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Minimal logical decoding on standbys |
Date: | 2021-03-23 17:31:48 |
Message-ID: | 96644a03-0b4d-e0a6-2124-0bc37e5b54fb@amazon.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v13-0005-Doc-changes-describing-details-about-logical-dec.patch | text/plain | 1.8 KB |
v13-0004-New-TAP-test-for-logical-decoding-on-standby.patch | text/plain | 20.9 KB |
v13-0003-Handle-logical-slot-conflicts-on-standby.patch | text/plain | 26.7 KB |
v13-0002-Add-info-in-WAL-records-in-preparation-for-logic.patch | text/plain | 19.7 KB |
v13-0001-Allow-logical-decoding-on-standby.patch | text/plain | 9.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2021-03-23 17:56:30 | Re: WIP: BRIN multi-range indexes |
Previous Message | Jan Wieck | 2021-03-23 17:25:15 | Re: pg_upgrade failing for 200+ million Large Objects |