Parallell hashjoin sometimes ignores temp_tablespaces
If a database (a) has a default tablespace set,
Reproduction:
CREATE TABLESPACE t LOCATION '/tmp/t';
CREATE DATABASE dumb TABLESPACE t;
\c dumb
SET temp_tablespaces=t;
At this point if you run a query with a parallel hash join in it, the
tempfiles go in base/pgsql_tmp instead of the temporary tablespace. For
example:
create table foo(bar int);
insert into foo select * from generate_series(1,1000000);
set parallel_tuple_cost =0;
set parallel_setup_cost =0;
set log_temp_files=0;
set client_min_messages ='log';
explain analyze select foo.bar,count(*) from foo inner join foo foo2 on
foo.bar=foo2.bar group by foo.bar;
Will trigger some temp files in the 't' tablespace and some in the
'pg_default' one.
I think the fix is the attached one (tested on version 11 which is what
$customer is using). To me it looks like this may have been a copy/paste
error all the way back in 98e8b480532 which added default_tablespace back
in 2004. (And is in itself entirely unrelated to parallel hashjoin, but
that's where it got exposed at least in my case)
Thoughts?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Attachments:
temp_tablespaces.patchtext/x-patch; charset=US-ASCII; name=temp_tablespaces.patchDownload+1-1
On 29 Jun 2020, at 17:02, Magnus Hagander <magnus@hagander.net> wrote:
I think the fix is the attached one (tested on version 11 which is what $customer is using). To me it looks like this may have been a copy/paste error all the way back in 98e8b480532 which added default_tablespace back in 2004. (And is in itself entirely unrelated to parallel hashjoin, but that's where it got exposed at least in my case)
Running through the repro and patch on HEAD I confirm that the attached fixes
the issue. +1 for the patch and a backpatch of it.
It would be nice to have a test covering test_tablespaces, but it seems a tad
cumbersome to create a stable one.
cheers ./daniel
On Mon, Jun 29, 2020 at 10:26 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 29 Jun 2020, at 17:02, Magnus Hagander <magnus@hagander.net> wrote:
I think the fix is the attached one (tested on version 11 which is what
$customer is using). To me it looks like this may have been a copy/paste
error all the way back in 98e8b480532 which added default_tablespace back
in 2004. (And is in itself entirely unrelated to parallel hashjoin, but
that's where it got exposed at least in my case)Running through the repro and patch on HEAD I confirm that the attached
fixes
the issue. +1 for the patch and a backpatch of it.It would be nice to have a test covering test_tablespaces, but it seems a
tad
cumbersome to create a stable one.
Thanks. pushed!
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Magnus Hagander <magnus@hagander.net> writes:
Thanks. pushed!
Sorry for not having paid more attention earlier, but this patch is
quite broken. If it weren't misguided it'd still be wrong, because
this isn't the only spot in PrepareTempTablespaces that inserts
InvalidOid into the output list.
But, in fact, it's intentional that we represent the DB's default
tablespace by InvalidOid in that list. Some callers of
GetNextTempTableSpace need that to be the case, either for
permissions-checking reasons or because they're going to store the
result into a temp table's pg_class.reltablespace, where that
representation is *required*.
I see that this is perhaps underdocumented, since while
GetNextTempTableSpace's comment mentions the behavior, there's
no comment about it with the data structure proper.
It looks to me like the actual bug here is that whoever added
GetTempTablespaces() and made sharedfileset.c depend on it
did not get the memo about what to do with InvalidOid.
It's possible that we could safely make GetTempTablespaces()
do the substitution, but that would be making fd.c assume more
about the usage of GetTempTablespaces() than I think it ought to.
I feel like we oughta fix sharedfileset.c, instead.
regards, tom lane
On Fri, Jul 3, 2020 at 4:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
Thanks. pushed!
Sorry for not having paid more attention earlier, but this patch is
quite broken. If it weren't misguided it'd still be wrong, because
this isn't the only spot in PrepareTempTablespaces that inserts
InvalidOid into the output list.But, in fact, it's intentional that we represent the DB's default
tablespace by InvalidOid in that list. Some callers of
GetNextTempTableSpace need that to be the case, either for
permissions-checking reasons or because they're going to store the
result into a temp table's pg_class.reltablespace, where that
representation is *required*.
Hmm. I guess I must've been careless in checking other callers.
I see that this is perhaps underdocumented, since while
GetNextTempTableSpace's comment mentions the behavior, there's
no comment about it with the data structure proper.
Yeah, it could definitely do with that. It was too many steps of
indirections away to me to pick that up.
It looks to me like the actual bug here is that whoever added
GetTempTablespaces() and made sharedfileset.c depend on it
did not get the memo about what to do with InvalidOid.
It's possible that we could safely make GetTempTablespaces()
do the substitution, but that would be making fd.c assume more
about the usage of GetTempTablespaces() than I think it ought to.
I feel like we oughta fix sharedfileset.c, instead.
This seems to be in dc6c4c9dc2a -- adding Thomas.
A quick look -- to do things right, we will need to know the database
default tablespace in this case right? Which I guess isn't there because
the shared fileset isn't tied to a database. But perhaps it's as easy as
something like the attached, just overwriting the oid?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Attachments:
temp_tablespaces_2.patchtext/x-patch; charset=US-ASCII; name=temp_tablespaces_2.patchDownload+19-0
Magnus Hagander <magnus@hagander.net> writes:
A quick look -- to do things right, we will need to know the database
default tablespace in this case right? Which I guess isn't there because
the shared fileset isn't tied to a database. But perhaps it's as easy as
something like the attached, just overwriting the oid?
Yeah, we just have to pick an appropriate place for making the
substitution. I have no objection to doing it in SharedFileSetInit, as
long as we're sure it will only be consulted for placing temp files and
not relations.
The lack of documentation seems to be my fault, so I'm willing to pick
this up unless somebody else wants it.
regards, tom lane
On Fri, Jul 3, 2020 at 6:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
A quick look -- to do things right, we will need to know the database
default tablespace in this case right? Which I guess isn't there because
the shared fileset isn't tied to a database. But perhaps it's as easy as
something like the attached, just overwriting the oid?Yeah, we just have to pick an appropriate place for making the
substitution. I have no objection to doing it in SharedFileSetInit, as
long as we're sure it will only be consulted for placing temp files and
not relations.
It doesn't *now*, and I'm pretty sure it can't be in the future the way it
is now (a parallel worker can't be creating relations). But it is probably
a good idea to add a comment indicating this as well...
The lack of documentation seems to be my fault, so I'm willing to pick
this up unless somebody else wants it.
If the comments I included in that patch are enough, I can just commit
those along with it. Otherwise, please do :)
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Magnus Hagander <magnus@hagander.net> writes:
On Fri, Jul 3, 2020 at 6:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
The lack of documentation seems to be my fault, so I'm willing to pick
this up unless somebody else wants it.
If the comments I included in that patch are enough, I can just commit
those along with it. Otherwise, please do :)
Being once burned, I had something more like the attached in mind.
BTW, looking at this, I'm kind of wondering about the other code path
in SharedFileSetInit:
if (fileset->ntablespaces == 0)
{
fileset->tablespaces[0] = DEFAULTTABLESPACE_OID;
fileset->ntablespaces = 1;
}
Shouldn't that be inserting MyDatabaseTableSpace? I see no other places
anywhere that are forcing temp stuff into pg_default like this.
regards, tom lane
Attachments:
temp_tablespaces_3.patchtext/x-diff; charset=us-ascii; name=temp_tablespaces_3.patchDownload+32-4
On Fri, Jul 3, 2020 at 7:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Fri, Jul 3, 2020 at 6:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
The lack of documentation seems to be my fault, so I'm willing to pick
this up unless somebody else wants it.If the comments I included in that patch are enough, I can just commit
those along with it. Otherwise, please do :)Being once burned, I had something more like the attached in mind.
That's a bit more elaborate and yes, I agree, better.
BTW, looking at this, I'm kind of wondering about the other code path
in SharedFileSetInit:
if (fileset->ntablespaces == 0)
{
fileset->tablespaces[0] = DEFAULTTABLESPACE_OID;
fileset->ntablespaces = 1;
}Shouldn't that be inserting MyDatabaseTableSpace? I see no other places
anywhere that are forcing temp stuff into pg_default like this.
Yeah, looking at it again, I think it should. I can't see any reason why it
should enforce pg_default.
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Magnus Hagander <magnus@hagander.net> writes:
On Fri, Jul 3, 2020 at 7:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Shouldn't that be inserting MyDatabaseTableSpace? I see no other places
anywhere that are forcing temp stuff into pg_default like this.
Yeah, looking at it again, I think it should. I can't see any reason why it
should enforce pg_default.
OK, pushed with that additional correction.
regards, tom lane
On Fri, Jul 3, 2020 at 11:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Fri, Jul 3, 2020 at 7:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Shouldn't that be inserting MyDatabaseTableSpace? I see no other places
anywhere that are forcing temp stuff into pg_default like this.Yeah, looking at it again, I think it should. I can't see any reason why
it
should enforce pg_default.
OK, pushed with that additional correction.
LGTM, thanks!
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>