Re: non-exclusive backup cleanup is mildly broken

At Tue, 17 Dec 2019 15:48:40 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> I wrote:
> > Took a quick look.  I agree that this seems a lot cleaner than the
> > alternative proposals.  I'd suggest however that the header comment
> > for do_pg_abort_backup could do with more work, perhaps along the
> > lines of "The odd-looking signature allows this to be registered
> > directly as a shmem_exit handler".
> 
> > Personally I'd have kept the handler as a separate function that's just
> > a one-line wrapper around "void do_pg_abort_backup(bool emit_warning)".
> > We don't usually treat callbacks as functions to be also called in
> > their own right.  But if you don't want to do that, I'll settle for an
> > acknowledgement of the hack in the comment.
> 
> Oh, scratch that --- looking closer, I see that the only two use-cases in
> the patched code are via before_shmem_exit and PG_ENSURE_ERROR_CLEANUP,
> and both of those require a function with the signature of an on_exit
> callback.  So there's no need for a separate wrapper because this isn't
> going to be called any other way.  I still recommend amending the
> comment to explain why it has this signature, though.

The existing comment of do_pg_abort_backup follows seems to be a bit
stale to me. I think it can be revised that way.

  * NB: This is only for aborting a non-exclusive backup that doesn't write
  * backup_label. A backup started with pg_start_backup() needs to be finished
  * with pg_stop_backup().


Don't we need a regression test for this behavior?


+ * Register a handler that will warn about unterminated backups at end of
+ * session, unless this has already been done.

Though I'm not sure of the necessity to do, it might need to mention
cleaning up, which is actually done by the function.


Other than the above looks good to me. The patch applies cleanly and
works as expected.


In another branch of this thread,

At Tue, 17 Dec 2019 08:38:13 -0500, Robert Haas <robertmhaas@gmail.com> wrote in 
> On the question of whether the assertion is needed, it is currently
> the case that you could call cancel_before_shmem_exit() without
> knowing whether you'd actually registered a handler or not. With the
> proposed change, that would no longer be legal. Maybe that's OK. But
> it doesn't seem entirely crazy to have some error-recovery path where
> cancel_before_shmem_exit() could get called twice in obscure
> circumstances, and right now, you can rest easy, knowing that the
> second call just won't do anything. If we make it an assertion failure
> to do such things, then you can't. On the other hand, maybe there's no
> use for such a construct, and it'd be better to just reduce confusion.
> Anyway, I think this is a separate topic from fixing this specific
> problem.

The part is not for the case "without knowing whether you'd actually
registered a handler or not", but for the case "without knowing
whether someone could have registered a handler or not after the
registration I made". But there is not so much difference and I don't
insist on that because of the reason you mentioned as above. Thanks
for elaborating.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center