Horton

Thread

Commits

  1. pgstatindex, pageinspect: handle partitioned indexes

  2. Add relkind 'I' to catalog.sgml's list relkinds

  1. Local partitioned indexes and pageinspect

    Peter Geoghegan <pg@bowt.ie> — 2018-04-24T00:29:59Z

    It looks like local partitioned indexes are a problem for pageinspect:

pg@regression[28736]=# select bt_metap('at_partitioned_b_idx');
ERROR:  relation "at_partitioned_b_idx" is not a btree index

I gather that pageinspect simply didn't get the memo about the new
RELKIND_PARTITIONED_INDEX "placeholder" relkind. I think that it
should at least give a friendlier error message than what we see here.
The same applies to amcheck, and possibly a few other modules.

I also noticed that the new 'I' relkind is not documented within the
pg_class docs. I think that that needs to be updated. The docs should
explain that 'I' relations are not backed by storage, since that will
probably affect users of one or two external tools.

-- 
Peter Geoghegan

  2. Re: Local partitioned indexes and pageinspect

    Michael Paquier <michael@paquier.xyz> — 2018-04-24T04:58:12Z

    On Mon, Apr 23, 2018 at 05:29:59PM -0700, Peter Geoghegan wrote:
> It looks like local partitioned indexes are a problem for pageinspect:
> 
> pg@regression[28736]=# select bt_metap('at_partitioned_b_idx');
> ERROR:  relation "at_partitioned_b_idx" is not a btree index

Okay, I can see the point you are making here, in this case that's
actually a btree index but it has no on-disk data, so let's improve the
error handling.  At the same time hash and gin functions don't make much
effort either...

> I gather that pageinspect simply didn't get the memo about the new
> RELKIND_PARTITIONED_INDEX "placeholder" relkind. I think that it
> should at least give a friendlier error message than what we see here.
> The same applies to amcheck, and possibly a few other modules.

So that's another c08d82f3...  This needs to be really checked on a
yearly-basis as the trend of the last releases is to add at least one
new relkind per major version.  There are three modules at stake here:
- pg_visibility
- pgstattuple
- pageinspect
I get to wonder about anything needed for sepgsql...  Let's see that on
a separate thread after I look at the problem.

> I also noticed that the new 'I' relkind is not documented within the
> pg_class docs. I think that that needs to be updated.

Good catch.

> The docs should
> explain that 'I' relations are not backed by storage, since that will
> probably affect users of one or two external tools.

Hm, the docs about taking backups with the low-level APIs don't care
much about relkind now:
https://www.postgresql.org/docs/devel/static/continuous-archiving.html#BACKUP-LOWLEVEL-BASE-BACKUP
Or do you have another section in the docs in mind?

Does the attached patch address everything you have seen?  I have tried
to cope with anything I noticed as well on the way, which is made of:
- pg_visibility should have tests for partitioned indexes, the code
handles errors correctly.
- in pageinspect, get_raw_page() fails for partitioned indexes:
ERROR:  could not open file "base/16384/16419": No such file or directory
- in pgstattuple, error message is incorrect for both index-related and
heap-related functions.

Thanks,
--
Michael

  3. Re: Local partitioned indexes and pageinspect

    Peter Geoghegan <pg@bowt.ie> — 2018-04-30T01:20:02Z

    On Mon, Apr 23, 2018 at 9:58 PM, Michael Paquier <michael@paquier.xyz> wrote:
> Hm, the docs about taking backups with the low-level APIs don't care
> much about relkind now:
> https://www.postgresql.org/docs/devel/static/continuous-archiving.html#BACKUP-LOWLEVEL-BASE-BACKUP
> Or do you have another section in the docs in mind?

No, I didn't. We should definitely document new relkinds in the
pg_class docs, though.

> Does the attached patch address everything you have seen?

What about amcheck? I did change the example query in the docs to
account for this, so anyone that generalizes from that won't have a
problem, but it would be nice if it had a friendlier message. I didn't
change amcheck to account for this myself because I thought it was
possible that somebody else would want to use a more general solution.

-- 
Peter Geoghegan

  4. Re: Local partitioned indexes and pageinspect

    Michael Paquier <michael@paquier.xyz> — 2018-04-30T06:50:13Z

    On Sun, Apr 29, 2018 at 06:20:02PM -0700, Peter Geoghegan wrote:
> What about amcheck? I did change the example query in the docs to
> account for this, so anyone that generalizes from that won't have a
> problem, but it would be nice if it had a friendlier message. I didn't
> change amcheck to account for this myself because I thought it was
> possible that somebody else would want to use a more general solution.

Perhaps it would help if an errhint is added which is written as "This
relation is a %s", where %s is a relkind converted to a translatable
string describing the kind?  All those modules work on different objects
and have different goals and prospoectives, so it looks difficult to me
to come up with a sane API which is rather portable across modules.

The statu-quo is not completely bad either (aka no extra error
messages) as incorrect relation kind are still filtered out, perhaps the
docs don't emphasize enough that an index and a partitioned index are
two different things?
--
Michael

  5. Re: Local partitioned indexes and pageinspect

    Robert Haas <robertmhaas@gmail.com> — 2018-05-01T16:30:44Z

    On Mon, Apr 30, 2018 at 2:50 AM, Michael Paquier <michael@paquier.xyz> wrote:
> On Sun, Apr 29, 2018 at 06:20:02PM -0700, Peter Geoghegan wrote:
>> What about amcheck? I did change the example query in the docs to
>> account for this, so anyone that generalizes from that won't have a
>> problem, but it would be nice if it had a friendlier message. I didn't
>> change amcheck to account for this myself because I thought it was
>> possible that somebody else would want to use a more general solution.
>
> Perhaps it would help if an errhint is added which is written as "This
> relation is a %s", where %s is a relkind converted to a translatable
> string describing the kind?  All those modules work on different objects
> and have different goals and prospoectives, so it looks difficult to me
> to come up with a sane API which is rather portable across modules.

That's probably going to cause some translation problems.  The form of
"a" that you need will vary: "a" vs. "an" in English, "un" vs. "una"
in Spanish, etc.  And it wouldn't be surprising if there are problems
in some languages even if you make it "This relation is %s".  There's
a reason why the documentation advises against building up
translatable strings from constituent parts.  If we go this route,
it's probably best to separately translate "This relation is a
table.", "This relation is an index.", etc.

However, backing up a minute, I don't think "relation \"%s\" is not a
btree index" is such a terrible message.  These modules are intended
to be intended by people who Know What They Are Doing.  If we do want
to change the message, I submit that the only thing that makes it a
little unclear is that a user might fail to realize that a partitioned
index is not an index.  But that could be fixed just by adding a
separate message for that one case (index \"%s\" is partitioned) and
sticking with the existing message for other cases.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  6. Re: Local partitioned indexes and pageinspect

    Michael Paquier <michael@paquier.xyz> — 2018-05-02T02:05:33Z

    On Tue, May 01, 2018 at 12:30:44PM -0400, Robert Haas wrote:
> That's probably going to cause some translation problems.  The form of
> "a" that you need will vary: "a" vs. "an" in English, "un" vs. "una"
> in Spanish, etc.  And it wouldn't be surprising if there are problems
> in some languages even if you make it "This relation is %s".  There's
> a reason why the documentation advises against building up
> translatable strings from constituent parts.  If we go this route,
> it's probably best to separately translate "This relation is a
> table.", "This relation is an index.", etc.

Yeah, I get the feeling that this is not going to be much
consistent-proof either, so while I have not been able to come up with a
better idea, let's not go through this route.

> However, backing up a minute, I don't think "relation \"%s\" is not a
> btree index" is such a terrible message.  These modules are intended
> to be intended by people who Know What They Are Doing.  If we do want
> to change the message, I submit that the only thing that makes it a
> little unclear is that a user might fail to realize that a partitioned
> index is not an index.  But that could be fixed just by adding a
> separate message for that one case (index \"%s\" is partitioned) and
> sticking with the existing message for other cases.

I have been chewing on that, and I come to agree that there is perhaps
little point to complicate the code as long as a failure is properly
reported to the user.  I propose hence the attached, which adds test
cases in the contrib module set for partitioned indexes (amcheck also
lacked tests for partition tables and indexes), and fixes a set of code
paths to be consistent with the presence of this new relkind.

Visibly I have found a bug in git, format-patch reports the following
line in the stats:
 .../pg_visibility/expected/pg_visibility.out    | 13 ++++++++++++-
But the patch contents are actually correct...
--
Michael

  7. Re: [SPAM] Re: Local partitioned indexes and pageinspect

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-02T04:38:22Z

    Hi.

On 2018/05/02 11:05, Michael Paquier wrote:
> On Tue, May 01, 2018 at 12:30:44PM -0400, Robert Haas wrote:
>> However, backing up a minute, I don't think "relation \"%s\" is not a
>> btree index" is such a terrible message.  These modules are intended
>> to be intended by people who Know What They Are Doing.  If we do want
>> to change the message, I submit that the only thing that makes it a
>> little unclear is that a user might fail to realize that a partitioned
>> index is not an index.  But that could be fixed just by adding a
>> separate message for that one case (index \"%s\" is partitioned) and
>> sticking with the existing message for other cases.

+1

> I have been chewing on that, and I come to agree that there is perhaps
> little point to complicate the code as long as a failure is properly
> reported to the user.  I propose hence the attached, which adds test
> cases in the contrib module set for partitioned indexes (amcheck also
> lacked tests for partition tables and indexes), and fixes a set of code
> paths to be consistent with the presence of this new relkind.

--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out

+-- verify partitioned tables are rejected (error)
+SELECT bt_index_check('bttest_partitioned');
+ERROR:  "bttest_partitioned" is not an index

Perhaps, I'm just repeating what's already been said, but I think it might
be better to have the word "partitioned" in the message.

ERROR: "bttest_partitioned" is partitioned index

..which Robert seems to think might not be too bad.

That will need adding some code to these modules like we did in
c08d82f38ebf763 [1].

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c08d82f38eb

  8. Re: Local partitioned indexes and pageinspect

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-02T04:41:38Z

    On 2018/05/02 13:38, Amit Langote wrote:
> --- a/contrib/amcheck/expected/check_btree.out
> +++ b/contrib/amcheck/expected/check_btree.out
> 
> +-- verify partitioned tables are rejected (error)
> +SELECT bt_index_check('bttest_partitioned');
> +ERROR:  "bttest_partitioned" is not an index
> 
> Perhaps, I'm just repeating what's already been said, but I think it might
> be better to have the word "partitioned" in the message.
> 
> ERROR: "bttest_partitioned" is partitioned index
> 
> ..which Robert seems to think might not be too bad.
> 
> That will need adding some code to these modules like we did in
> c08d82f38ebf763 [1].

Sorry, it appears that our mail system added "[SPAM]" to the subject-line
for one reason or another, which I forgot to manually remove when editing
the email.

- Amit

  9. Re: [SPAM] Re: Local partitioned indexes and pageinspect

    Michael Paquier <michael@paquier.xyz> — 2018-05-02T06:14:52Z

    On Wed, May 02, 2018 at 01:38:22PM +0900, Amit Langote wrote:
> Perhaps, I'm just repeating what's already been said, but I think it might
> be better to have the word "partitioned" in the message.

That's what Peter is pointing to upthread and what the v1 of upthread
was doing.  I would tend to think to just keep the code simple and don't
add those extra checks, but I am fine to be beaten as well.
--
Michael

  10. Re: [SPAM] Re: Local partitioned indexes and pageinspect

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-05-09T17:28:50Z

    Michael Paquier wrote:
> On Wed, May 02, 2018 at 01:38:22PM +0900, Amit Langote wrote:
> > Perhaps, I'm just repeating what's already been said, but I think it might
> > be better to have the word "partitioned" in the message.
> 
> That's what Peter is pointing to upthread and what the v1 of upthread
> was doing.  I would tend to think to just keep the code simple and don't
> add those extra checks, but I am fine to be beaten as well.

I pushed some fixes produced here.  Attached is the remainder of the
patch you submitted.  I notice now that we haven't actually fixed
Peter's source of complaint, though.  AFAICS your patch just adds test
cases, and upthread discussion apparently converges on not doing
anything about the code.  I'm not yet sure what to think of that ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: [SPAM] Re: Local partitioned indexes and pageinspect

    Michael Paquier <michael@paquier.xyz> — 2018-05-09T21:04:26Z

    On Wed, May 09, 2018 at 02:28:50PM -0300, Alvaro Herrera wrote:
> I pushed some fixes produced here.  Attached is the remainder of the
> patch you submitted.  I notice now that we haven't actually fixed
> Peter's source of complaint, though.  AFAICS your patch just adds test
> cases, and upthread discussion apparently converges on not doing
> anything about the code.  I'm not yet sure what to think of that ...

Thanks Álvaro.  I tend to think that there is little point in changing
the error handling, still it would be good to get test coverage.  That's
not really a bug though as in all those cases we don't get errors like
"could not open file" or such.  So we could also let things as they are
now.
--
Michael

  12. Re: [SPAM] Re: Local partitioned indexes and pageinspect

    Peter Geoghegan <pg@bowt.ie> — 2018-05-09T21:05:47Z

    On Wed, May 9, 2018 at 2:04 PM, Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, May 09, 2018 at 02:28:50PM -0300, Alvaro Herrera wrote:
>> I pushed some fixes produced here.  Attached is the remainder of the
>> patch you submitted.  I notice now that we haven't actually fixed
>> Peter's source of complaint, though.  AFAICS your patch just adds test
>> cases, and upthread discussion apparently converges on not doing
>> anything about the code.  I'm not yet sure what to think of that ...
>
> Thanks Álvaro.  I tend to think that there is little point in changing
> the error handling, still it would be good to get test coverage.  That's
> not really a bug though as in all those cases we don't get errors like
> "could not open file" or such.  So we could also let things as they are
> now.

Now that the relkind issue is documented, I wouldn't mind just leaving it as-is.

-- 
Peter Geoghegan