Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> Apart from the invalid snapshot problem, I looked the patch
> previously mentioned mainly for Windows.
Thanks for looking!
> Even though the threads started by beginthread cannot be
> terminated cleanly from outside, but the whole process will soon
> terminate anyway, so we could use TreminateThread. This seems
> working. (Attached patch)
Seems reasonable to me; I was unhappy about the lack of any direct
equivalent to the child SIGTERMs that the Unix code does.
> Is there any reason for the name "ourAH" not to be "myAH"?
Don't much care, I'll change it.
> setup_cancel_handler looks somewhat bizarre. It eventually works
> only for the main process/thread and does nothing for workers. It
> is enough to be run once before forking in ParalleBackupStart and
> that makes handler_set unnecessary.
No, because we also want it to work in non-parallel cases. As coded,
we'll establish the handler whenever set_archive_cancel_info is first
called, which will be in the first ConnectDatabase() call. It would
be possible to do that someplace else maybe, but it would require
more code changes than just attaching it to set_archive_cancel_info.
> In EndDBCopyMode, the result of PQgetResult is abandoned. This
> can leak memory and such usage is not seen elsewhere in the
> source tree. Shouldn't we hold the result and PQclear it? (Mainly
> as a convention, not for benefit.)
There should never be any non-null result, so that seems like a waste
of code to me.
regards, tom lane