Subject: Re: contrib modules and relkind check

From: Amit Langote <[email-masked]>
Date: Tue, 7 Mar 2017 16:15:32 +0900

Sorry about the absence on this thread.

On 2017/02/14 15:30, Michael Paquier wrote:
> On Tue, Feb 14, 2017 at 3:18 PM, Amit Langote wrote:
>>
>> Added more tests in pgstattuple and the new ones for pg_visibility,
>> although I may have overdone the latter.
> 
> A bonus idea is also to add tests for relkinds that work, with for
> example the creation of a table, inserting some data in it, vacuum it,
> and look at "SELECT count(*) > 0 FROM pg_visibility('foo'::regclass)".

I assume you meant only for pg_visibility.  Done in the attached (a pretty
basic test though).

>> In certain contexts where a subset of relkinds are allowed and others are
>> not or vice versa, partitioned tables are still referred to as "tables".
>> That's because we still use CREATE/DROP TABLE to create/drop them and
>> perhaps more to the point, their being partitioned is irrelevant.
>>
>> Examples of where partitioned tables are referred to as tables:
>>
>> [...]
>>
>> In other contexts, where a table's being partitioned is relevant, the
>> message is shown as "relation is/is not partitioned table".  Examples:
>>
>> [...]
> 
> Hm... It may be a good idea to be consistent on the whole system and
> refer to "partitioned table" as a table without storage and used as an
> entry point for partitions. The docs use this term in CREATE TABLE,
> and we would finish with messages like "not a table or a partitioned
> table". Extra thoughts are welcome here, the current inconsistencies
> would be confusing for users.

If we decide to go with some different approach, we'd not be doing it
here.  Maybe in the "partitioned tables and relfilenode" thread or a new one.

Thanks,
Amit

---

From: Michael Paquier <[email-masked]>
Date: Wed, 8 Mar 2017 16:47:26 +0900

> Sorry about the absence on this thread.

No problems! Thanks for showing up with an updated patch.

> I assume you meant only for pg_visibility.  Done in the attached (a pretty

Yep.

> here.  Maybe in the "partitioned tables and relfilenode" thread or a new one.

Okay.

+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -0,0 +1,85 @@
+CREATE EXTENSION pg_visibility;
+--
+-- check that using the module's functions with unsupported relations will fail
+--
[...]
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column?
+----------
+ t
+(1 row)
Only regular tables are tested as valid objects. Testing toast tables
is not worth the complication. Could you add as well a matview?

Except for this small issue the patch looks good to me.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

---

From: Amit Langote <[email-masked]>
Date: Wed, 8 Mar 2017 17:10:21 +0900

On 2017/03/08 16:47, Michael Paquier wrote:
> On Tue, Mar 7, 2017 at 4:15 PM, Amit Langote wrote:

Done in the attached updated patch.

> Except for this small issue the patch looks good to me.

Thanks.

Regards,
Amit

---

From: Michael Paquier <[email-masked]>
Date: Thu, 9 Mar 2017 11:51:25 +0900

> Done in the attached updated patch.

+select count(*) > 0 from pg_visibility('matview');
+ ?column?
+----------
+ f
+(1 row)
That's quite a generic name :) You may want to use less common names
in your tests.

OK, I am marking that as ready for committer.
-- 
Michael

---

From: Amit Langote <[email-masked]>
Date: Thu, 9 Mar 2017 12:02:10 +0900

> +select count(*) > 0 from pg_visibility('matview');

I agree.  Updated patch attached.

Thanks.

Regards,
Amit

---

From: Stephen Frost <[email-masked]>
Date: Thu, 9 Mar 2017 08:25:38 -0500

Amit, Michael,

> > OK, I am marking that as ready for committer.

I'm starting to look over this, seems pretty straight-forward.

Barring issues, I should get it committed in probably an hour or two.

Thanks!

Stephen

---

From: Stephen Frost <[email-masked]>
Date: Thu, 9 Mar 2017 16:48:49 -0500

Amit, Michael,

> > OK, I am marking that as ready for committer.

Thanks for this, I've pushed this now.  I do have a few notes about
changes that I made from your patch;

- Generally speaking, the user-facing functions come first in these .c
  files, with a prototype at the top for the static functions defined
  later on in the file.  I went ahead and did that for the functions you
  added too.

- I added more comments to the regression tests, in particular, we
  usually comment when tests are expected to fail.

- I added some additional regression tests to cover more cases,
  particularly ones for things that weren't being tested at all.

- Not the fault of your patch, but there were cases where elog() was
  being used when it really should have been ereport(), so I changed
  those cases to all be, hopefully, consistent throughout.

Thanks again!

Stephen

---

From: Amit Langote <[email-masked]>
Date: Fri, 10 Mar 2017 08:59:23 +0900

Hi Stephen,

>   those cases to all be, hopefully, consistent throughout.

Thanks a lot for all the improvements and committing.

Thanks,
Amit

---

From: Michael Paquier <[email-masked]>
Date: Fri, 10 Mar 2017 09:23:43 +0900

> Thanks a lot for all the improvements and committing.

Thanks. Shouldn't this fix be back-patched? pg_visibility should fail
properly for indexes and other relkinds even in 9.6. pgstattuple can
also trigger failures. It would be confusing for users to face "could
not open file" kind of errors instead of a proper error message. Note
that I am fine to produce those patches if there is a resource issue
for any of you two.

+-- an actual index of a partitiond table should work though
Typo here => s/partitiond/partitioned/
-- 
Michael

---

From: Stephen Frost <[email-masked]>
Date: Thu, 9 Mar 2017 19:34:45 -0500

Michael,

> that I am fine to produce those patches if there is a resource issue

I'm not really convinced that back-patching this is worthwhile, which is
why I didn't go through the effort to do so.  A reasonable error will be
thrown in either case, after all, without any bad behavior happening,
from what I can see.

That said, if you feel strongly enough about it to propose appropriate
patches for the back-branches, I'll try and look at them before the next
set of point releases, but I'm not going to deal with them this month as
I'd like to get through as much of the CF for PG10 as we can.

> Typo here => s/partitiond/partitioned/

Will fix.

Thanks!

Stephen

---

From: Michael Paquier <[email-masked]>
Date: Fri, 10 Mar 2017 09:53:55 +0900

> thrown in either case, after all, without any bad behavior happening,

Okay, I don't mind as nobody has complained about pgstattuple in particular.

> I'd like to get through as much of the CF for PG10 as we can.

Nah, let's see if somebody else complains.
-- 
Michael

---

From: Robert Haas <[email-masked]>
Date: Thu, 9 Mar 2017 21:03:15 -0500

> that I am fine to produce those patches if there is a resource issue

I don't see a real need to back-patch this.  I mean, it probably
wouldn't break anything, but it feels more like we're raising our
standards than fixing bugs per se.  I don't think it benefits us when
users see the release notes for a new minor release series cluttered
with essentially non-critical fixes.  It makes people worry about
whether upgrading is safe.  You might brand those people as silly, but
they (indirectly) pay my mortgage.

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