Re: Synchronizing slots from primary to standby

Hi,

On Thu, Feb 22, 2024 at 12:16:34PM +0530, shveta malik wrote:
> On Thu, Feb 22, 2024 at 10:31 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> There was a recent commit 801792e to improve error messaging in
> slotsync.c which resulted in conflict. Thus rebased the patch. There
> is no new change in the patch attached

Thanks!

Some random comments about v92_001 (Sorry if it has already been discussed
up-thread):

1 ===

+ * We do not update the 'synced' column from true to false here

Worth to mention from which system view the 'synced' column belongs to?

2 === (Nit)

+#define MIN_WORKER_NAPTIME_MS 200
+#define MAX_WORKER_NAPTIME_MS 30000 /* 30s */

[MIN|MAX]_SLOTSYNC_WORKER_NAPTIME_MS instead? It is used only in slotsync.c so
more a Nit.

3 ===

res = walrcv_exec(wrconn, query, SLOTSYNC_COLUMN_COUNT, slotRow);
-
if (res->status != WALRCV_OK_TUPLES)

Line removal intended?

4 ===

+ if (wal_level < WAL_LEVEL_LOGICAL)
+ {
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("slot synchronization requires wal_level >= \"logical\""));
+ return false;
+ }

I think the return is not needed here as it won't be reached due to the "ERROR".
Or should we use "elevel" instead of "ERROR"?

5 ===

+ * operate as a superuser. This is safe because the slot sync worker does
+ * not interact with user tables, eliminating the risk of executing
+ * arbitrary code within triggers.

Right. I did not check but if we are using operators in our remote SPI calls
then it would be worth to ensure they are coming from the pg_catalog schema?
Using something like "OPERATOR(pg_catalog.=)" using "=" as an example.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com