Re: Fixing order of resowner cleanup in 12, for Windows

On Mon, May 6, 2019 at 3:43 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Fri, May 3, 2019 at 2:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> > > A while back I posted a patch[1] to change the order of resowner
> > > cleanup so that DSM handles are released last. That's useful for the
> > > error cleanup path on Windows, when a SharedFileSet is cleaned up (a
> > > mechanism that's used by parallel CREATE INDEX and parallel hash join,
> > > for spilling files to disk under a temporary directory, with automatic
> > > cleanup).
> >
> > I guess what I'm wondering is if there are any potential negative
> > consequences, ie code that won't work if we change the order like this.
> > I'm finding it hard to visualize what that would be, but then again
> > this failure mode wasn't obvious either.
>
> I can't think of anything in core. The trouble here is that we're
> talking about hypothetical out-of-tree code that could want to plug in
> detach hooks to do anything at all, so it's hard to say. One idea
> that occurred to me is that if someone comes up with a genuine need to
> run arbitrary callbacks before locks are released (for example), we
> could provide a way to be called in all three phases and receive the
> phase, though admittedly in this case FileClose() is in the same phase
> as I'm proposing to put dsm_detach(), so there is an ordering
> requirement that might require more fine grained phases. I don't
> know.
>
> > > I suppose we probably should make the change to 12 though: then owners
> > > of extensions that use DSM detach hooks (if there any such extensions)
> > > will have a bit of time to get used to the new order during the beta
> > > period. I'll need to find someone to test this with a fault injection
> > > scenario on Windows before committing it, but wanted to sound out the
> > > list for any objections to this late change?
> >
> > Since we haven't started beta yet, I don't see a reason not to change
> > it. Worst case is that it causes problems and we revert it.
> >
> > I concur with not back-patching, in any case.
>
> Here's a way to produce an error which might produce the log message
> on Windows. Does anyone want to try it?
>

I can give it a try.

> postgres=# create table foo as select generate_series(1, 10000000)::int i;
> SELECT 10000000
> postgres=# set synchronize_seqscans = off;
> SET
> postgres=# create index on foo ((1 / (5000000 - i)));
> psql: ERROR: division by zero
> postgres=# create index on foo ((1 / (5000000 - i)));
> psql: ERROR: division by zero
> postgres=# create index on foo ((1 / (5000000 - i)));
> psql: ERROR: division by zero
> CONTEXT: parallel worker
>
> (If you don't turn sync scan off, it starts scanning from where it
> left off last time and then fails immediately, which may interfere
> with the experiment if you run it more than once, I'm not sure).
>
> If it does produce the log message, then the attached patch should
> make it go away.
>

Are you referring to log message "LOG: could not rmdir directory
"base/pgsql_tmp/pgsql_tmp3692.0.sharedfileset": Directory not empty"?
If so, I am getting it both before and after your patch.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com