Thread: Local partitioned indexes and pageinspect
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
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
Attachment
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
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
Attachment
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
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
Attachment
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
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
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
Attachment
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
Attachment
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
Attachment
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