Re: Libpq support to connect to standby server as priority - Mailing list pgsql-hackers

From Haribabu Kommi
Subject Re: Libpq support to connect to standby server as priority
Date
Msg-id CAJrrPGdUoxFo-QdtdjPF=_x9nYScZnS8OTu_1SOdfNfBsvXQOw@mail.gmail.com
Whole thread Raw
In response to RE: Libpq support to connect to standby server as priority  ("Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com>)
Responses Re: Libpq support to connect to standby server as priority
List pgsql-hackers

On Fri, Mar 29, 2019 at 7:06 PM Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote:
Hi Hari-san,

I've reviewed all the files.  The patch would be OK when the following have been fixed, except for the complexity of fe-connect.c (which probably cannot be improved.)

Unfortunately, I'll be absent next week.  The earliest date I can do the test will be April 8 or 9.  I hope someone could take care of this patch...

Thanks for the review. Apologies that I could not able finish it on time because of 
other work.

 

(1) 0001
With this read-only option type, application can connect to
to a read-only server in the list of hosts, in case
...
before issuing the SHOW transaction_readonly to find out whether


"to" appears twice in a row.
transaction_readonly -> transaction_read_only


(2) 0001
+        succesful connection or failure.

succesful -> successful


(3) 0008
to conenct to a standby server with a faster check instead of

conenct -> connect


(4) 0008
Logically, recovery exit should be notified after the following statement:

    XLogCtl->SharedRecoveryInProgress = false;


(5) 0008
+               /* Update in_recovery status. */
+               if (LocalRecoveryInProgress)
+                       SetConfigOption("in_recovery",
+                                                       "on",
+                                                       PGC_INTERNAL, PGC_S_OVERRIDE);
+

This SetConfigOption() is called for every RecoveryInProgress() call on the standby.  It may cause undesirable overhead.  How about just calling SetConfigOption() once in InitPostgres() to set the initial value for in_recovery?  InitPostgres() and its subsidiary functions call SetConfigOption() likewise.


(6) 0008
async.c is for LISTEN/UNLISTEN/NOTIFY.  How about adding the new functions in postgres.c like RecoveryConflictInterrupt()?


(7) 0008
+               if (pid != 0)
+               {
+                       (void) SendProcSignal(pid, reason, procvxid.backendId);
+               }

The braces are not necessary because the block only contains a single statement.

I fixed all the comments that you have raised above and attached the updated
patches.

Regards,
Haribabu Kommi
Fujitsu Australia
Attachment

pgsql-hackers by date:

Previous
From: Peter Geoghegan
Date:
Subject: Re: Reducing the runtime of the core regression tests
Next
From: Thomas Munro
Date:
Subject: Re: serializable transaction: exclude constraint violation (backed byGIST index) instead of ssi conflict