Thread: pg_tablespace_location() error message
The new pg_tablespace_location() function added in PG 9.2 to remove the director location from pg_tablespace returns an odd error for '0', which is InvalidOID: test=> select pg_tablespace_location(0);ERROR: could not read symbolic link "pg_tblspc/0": No such file ordirectory Is this OK? It handles NULL just fine:test=> select pg_tablespace_location(null); pg_tablespace_location------------------------(1row) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> The new pg_tablespace_location() function added in PG 9.2 to remove the
> director location from pg_tablespace returns an odd error for '0', which
> is InvalidOID:
Well, it's the same "odd error" you'd get for any other bogus OID.
The way the function is coded, it has no need to look into pg_tablespace
as such, which is why you don't get something like "no such tablespace".
We could add such a lookup purely for error detection purposes, but I'm
not real sure I see the point.
> Is this OK? It handles NULL just fine:
That's a consequence of the function being marked strict; it has nothing
much to do with anything.
regards, tom lane
On Tue, Apr 10, 2012 at 05:43:12PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > The new pg_tablespace_location() function added in PG 9.2 to remove the > > director location from pg_tablespace returns an odd error for '0', which > > is InvalidOID: > > Well, it's the same "odd error" you'd get for any other bogus OID. > > The way the function is coded, it has no need to look into pg_tablespace > as such, which is why you don't get something like "no such tablespace". > We could add such a lookup purely for error detection purposes, but I'm > not real sure I see the point. > > > Is this OK? It handles NULL just fine: > > That's a consequence of the function being marked strict; it has nothing > much to do with anything. OK, just checking before this function gets into a release. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Tue, Apr 10, 2012 at 5:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Bruce Momjian <bruce@momjian.us> writes: >> The new pg_tablespace_location() function added in PG 9.2 to remove the >> director location from pg_tablespace returns an odd error for '0', which >> is InvalidOID: > > Well, it's the same "odd error" you'd get for any other bogus OID. > > The way the function is coded, it has no need to look into pg_tablespace > as such, which is why you don't get something like "no such tablespace". > We could add such a lookup purely for error detection purposes, but I'm > not real sure I see the point. I think what Bruce might be getting at is that 0 is more likely than a randomly chosen value to be passed to this function; for example, one can imagine wanting to pass pg_class.reltablespace. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Apr 10, 2012 at 06:16:31PM -0400, Robert Haas wrote: > On Tue, Apr 10, 2012 at 5:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Bruce Momjian <bruce@momjian.us> writes: > >> The new pg_tablespace_location() function added in PG 9.2 to remove the > >> director location from pg_tablespace returns an odd error for '0', which > >> is InvalidOID: > > > > Well, it's the same "odd error" you'd get for any other bogus OID. > > > > The way the function is coded, it has no need to look into pg_tablespace > > as such, which is why you don't get something like "no such tablespace". > > We could add such a lookup purely for error detection purposes, but I'm > > not real sure I see the point. > > I think what Bruce might be getting at is that 0 is more likely than a > randomly chosen value to be passed to this function; for example, one > can imagine wanting to pass pg_class.reltablespace. Yes, that was my point. In tracking down a pg_upgrade bug, I discovered that zero means the cluser default location, while pg_tablespace_location() returning '' means the default _database_ (or global) tablespace. We are quite unclear on what DEFAULTTABLESPACE_OID means (the database default). -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Apr 10, 2012 at 06:16:31PM -0400, Robert Haas wrote:
>> On Tue, Apr 10, 2012 at 5:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> The way the function is coded, it has no need to look into pg_tablespace
>>> as such, which is why you don't get something like "no such tablespace".
>> I think what Bruce might be getting at is that 0 is more likely than a
>> randomly chosen value to be passed to this function; for example, one
>> can imagine wanting to pass pg_class.reltablespace.
> Yes, that was my point.
Hm. I have no objection to special-casing zero here, but what behavior
do you want? Should it return an empty string as we do for
DEFAULTTABLESPACE_OID, or throw a different error?
regards, tom lane
On Tue, Apr 10, 2012 at 07:09:33PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, Apr 10, 2012 at 06:16:31PM -0400, Robert Haas wrote: > >> On Tue, Apr 10, 2012 at 5:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> The way the function is coded, it has no need to look into pg_tablespace > >>> as such, which is why you don't get something like "no such tablespace". > > >> I think what Bruce might be getting at is that 0 is more likely than a > >> randomly chosen value to be passed to this function; for example, one > >> can imagine wanting to pass pg_class.reltablespace. > > > Yes, that was my point. > > Hm. I have no objection to special-casing zero here, but what behavior > do you want? Should it return an empty string as we do for > DEFAULTTABLESPACE_OID, or throw a different error? I have no idea. The big problem is that we currently use '' for the cluster default, while 0 means the database default. I can't think of a good return result --- I think it has to be an error of some kind. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Apr 10, 2012 at 07:09:33PM -0400, Tom Lane wrote:
>> Hm. I have no objection to special-casing zero here, but what behavior
>> do you want? Should it return an empty string as we do for
>> DEFAULTTABLESPACE_OID, or throw a different error?
> I have no idea. The big problem is that we currently use '' for the
> cluster default, while 0 means the database default. I can't think of a
> good return result --- I think it has to be an error of some kind.
If we expect this function to mainly be applied to pg_class.reltablespace,
then it seems like it ought to understand that zero means "the database
default" and substitute the database's default tablespace. That might
or might not be the same as the cluster default.
Alternatively, we could expect pg_upgrade to understand that and make
the substitution itself, but if the same would be needed by most uses of
the function, maybe we should just do it here.
regards, tom lane
On Tue, Apr 10, 2012 at 07:57:30PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, Apr 10, 2012 at 07:09:33PM -0400, Tom Lane wrote: > >> Hm. I have no objection to special-casing zero here, but what behavior > >> do you want? Should it return an empty string as we do for > >> DEFAULTTABLESPACE_OID, or throw a different error? > > > I have no idea. The big problem is that we currently use '' for the > > cluster default, while 0 means the database default. I can't think of a > > good return result --- I think it has to be an error of some kind. > > If we expect this function to mainly be applied to pg_class.reltablespace, > then it seems like it ought to understand that zero means "the database > default" and substitute the database's default tablespace. That might > or might not be the same as the cluster default. Well, do we really want to be reporting the _current_ data directory location? We do track tablespace symlink moves because we read the symlinks now, so that isn't out of the question. A bigger question is whether returning '' for a database-default location is valid --- I am thinking no. > Alternatively, we could expect pg_upgrade to understand that and make > the substitution itself, but if the same would be needed by most uses of > the function, maybe we should just do it here. I just applied a patch to pg_upgrade to do exactly that, and it is needed in pre-9.2 as well because pg_upgrade was testing for an empty spclocation, which could be '' or it could be NULL (from an outer join), but pg_upgrade wasn't distinguising the two. :-( -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Apr 10, 2012 at 07:57:30PM -0400, Tom Lane wrote:
>> If we expect this function to mainly be applied to pg_class.reltablespace,
>> then it seems like it ought to understand that zero means "the database
>> default" and substitute the database's default tablespace. That might
>> or might not be the same as the cluster default.
> Well, do we really want to be reporting the _current_ data directory
> location? We do track tablespace symlink moves because we read the
> symlinks now, so that isn't out of the question. A bigger question is
> whether returning '' for a database-default location is valid --- I am
> thinking no.
Well, the point is that we should return that for the *cluster* default
tablespace (mainly because we have nothing better available). But the
database default might or might not be the cluster default.
>> Alternatively, we could expect pg_upgrade to understand that and make
>> the substitution itself, but if the same would be needed by most uses of
>> the function, maybe we should just do it here.
> I just applied a patch to pg_upgrade to do exactly that,
AFAICS your patch does not avoid the fact that the function will throw
error on a zero input.
regards, tom lane
I wrote:
> AFAICS your patch does not avoid the fact that the function will throw
> error on a zero input.
Oh, no, scratch that --- I was thinking that you were applying the
function directly to pg_class.reltablespace, but you're applying it to
pg_tablespace.oid, so the issue should not arise.
regards, tom lane
On Tue, Apr 10, 2012 at 08:22:15PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, Apr 10, 2012 at 07:57:30PM -0400, Tom Lane wrote: > >> If we expect this function to mainly be applied to pg_class.reltablespace, > >> then it seems like it ought to understand that zero means "the database > >> default" and substitute the database's default tablespace. That might > >> or might not be the same as the cluster default. > > > Well, do we really want to be reporting the _current_ data directory > > location? We do track tablespace symlink moves because we read the > > symlinks now, so that isn't out of the question. A bigger question is > > whether returning '' for a database-default location is valid --- I am > > thinking no. > > Well, the point is that we should return that for the *cluster* default > tablespace (mainly because we have nothing better available). But the > database default might or might not be the cluster default. Right. The complexity is that in our pre-9.2 pg_tablespace, spclocation of '' means cluster default, and no row pg_tablespace row (0) meant db-default. The fact that pg_tablespace_location() returns '' for cluster default could be called an bug, or not. ;-) I think the functions purpose is just different from what pg_tablespace did -- the function returns the symlink location, so '' for cluster default seems fine because there is no symlink. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Tue, Apr 10, 2012 at 7:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > If we expect this function to mainly be applied to pg_class.reltablespace, > then it seems like it ought to understand that zero means "the database > default" and substitute the database's default tablespace. That might > or might not be the same as the cluster default. > > Alternatively, we could expect pg_upgrade to understand that and make > the substitution itself, but if the same would be needed by most uses of > the function, maybe we should just do it here. +1 for doing it in the function. I think that will improve ease-of-use and give up nothing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 10, 2012 at 7:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If we expect this function to mainly be applied to pg_class.reltablespace,
>> then it seems like it ought to understand that zero means "the database
>> default" and substitute the database's default tablespace. That might
>> or might not be the same as the cluster default.
>>
>> Alternatively, we could expect pg_upgrade to understand that and make
>> the substitution itself, but if the same would be needed by most uses of
>> the function, maybe we should just do it here.
> +1 for doing it in the function. I think that will improve
> ease-of-use and give up nothing.
I'm inclined to agree. It won't help pg_upgrade, but that's because
the query pg_upgrade is using is constrained by backwards-compatibility
considerations: it's *necessary* to join to pg_tablespace if you want
any location details pre-9.2. But as of 9.2 it's plausible to consider
applying this function directly to pg_class.reltablespace, so we should
make it behave sanely for that use-case.
I think it's probably about a two-line change, will research and
apply if so.
regards, tom lane
On Tue, Apr 10, 2012 at 09:19:05PM -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Tue, Apr 10, 2012 at 7:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> If we expect this function to mainly be applied to pg_class.reltablespace, > >> then it seems like it ought to understand that zero means "the database > >> default" and substitute the database's default tablespace. That might > >> or might not be the same as the cluster default. > >> > >> Alternatively, we could expect pg_upgrade to understand that and make > >> the substitution itself, but if the same would be needed by most uses of > >> the function, maybe we should just do it here. > > > +1 for doing it in the function. I think that will improve > > ease-of-use and give up nothing. > > I'm inclined to agree. It won't help pg_upgrade, but that's because > the query pg_upgrade is using is constrained by backwards-compatibility > considerations: it's *necessary* to join to pg_tablespace if you want > any location details pre-9.2. But as of 9.2 it's plausible to consider > applying this function directly to pg_class.reltablespace, so we should > make it behave sanely for that use-case. > > I think it's probably about a two-line change, will research and > apply if so. Agreed, thanks. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +