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.malik@gmail.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