Re: base backup client as auxiliary backend process

On Thu, 16 Jan 2020 at 00:17, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> On 2020-01-15 01:40, Masahiko Sawada wrote:
> > Could you rebase the main patch that adds base backup client as
> > auxiliary backend process since the previous version patch (v3)
> > conflicts with the current HEAD?
>
> attached

Thanks. I used and briefly looked at this patch. Here are some comments:

1.
+ /*
+ * Wait until done. Start WAL receiver in the meantime, once base
+ * backup has received the starting position.
+ */
+ while (BaseBackupPID != 0)
+ {
+ PG_SETMASK(&UnBlockSig);
+ pg_usleep(1000000L);
+ PG_SETMASK(&BlockSig);
+ MaybeStartWalReceiver();
+ }

Since the postmaster is sleeping the new connection hangs without any
message whereas normally we can get the message like "the database
system is starting up" during not accepting new connections. I think
some programs that checks the connectivity of PostgreSQL starting up
might not work fine with this. So many we might want to refuse all new
connections while waiting for taking basebackup.

2.
+ initStringInfo(&stmt);
+ appendStringInfo(&stmt, "BASE_BACKUP PROGRESS NOWAIT EXCLUDE_CONF");
+ if (cluster_name && cluster_name[0])

While using this patch I realized that the standby server cannot start
when the master server has larger value of some GUC parameter such as
max_connections and max_prepared_transactions than the default values.
And unlike taking basebackup using pg_basebacup or other methods the
database cluster initialized by this feature use default values for
all configuration parameters regardless of values in the master. So I
think it's better to include .conf files but we will end up with
overwriting the local .conf files instead. So I thought that
basebackup process can fetch .conf files from the master server and
add primary_conninfo to postgresql.auto.conf but I'm not sure.

3.
+ if (stat(BASEBACKUP_SIGNAL_FILE, &stat_buf) == 0)
+ {
+ int fd;
+
+ fd = BasicOpenFilePerm(STANDBY_SIGNAL_FILE, O_RDWR | PG_BINARY,
+ S_IRUSR | S_IWUSR);
+ if (fd >= 0)
+ {
+ (void) pg_fsync(fd);
+ close(fd);
+ }
+ basebackup_signal_file_found = true;
+ }
+

Why do we open and just close the file?

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services