pgrowlocks relkind check

Amit,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:

The following commit added relkind checks to certain contrib modules so
that a more user-friendly error is produced if the wrong kind of relation
is passed to its functions:

commit c08d82f38ebf763b79bd43ae34b7310ee47aaacd
Author: Stephen Frost <sfrost@snowman.net>
Date: Thu Mar 9 16:34:25 2017 -0500

Add relkind checks to certain contrib modules

But it missed pgrowlocks, so the following happens:

create extension pgrowlocks;
create view one as select 1;
select pgrowlocks('one');
-- ERROR: could not open file "base/68730/68748": No such file or directory

With the attached patch:

select pgrowlocks('one');
ERROR: "one" is not a table, index, materialized view, sequence, or TOAST
table

Good point.

Thanks, I'll see about committing this shortly.

Stephen

Hi Stephen,

On 2017/04/11 22:17, Stephen Frost wrote:

create extension pgrowlocks;
create view one as select 1;
select pgrowlocks('one');
-- ERROR: could not open file "base/68730/68748": No such file or directory

With the attached patch:

select pgrowlocks('one');
ERROR: "one" is not a table, index, materialized view, sequence, or TOAST
table

Good point.

Thanks, I'll see about committing this shortly.

Should I add this to the next commitfest (not an open item per se) or will
you be committing it?

Thanks,
Amit

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

On 4/24/17 21:22, Amit Langote wrote:

Hi Stephen,

On 2017/04/11 22:17, Stephen Frost wrote:

create extension pgrowlocks;
create view one as select 1;
select pgrowlocks('one');
-- ERROR: could not open file "base/68730/68748": No such file or directory

With the attached patch:

select pgrowlocks('one');
ERROR: "one" is not a table, index, materialized view, sequence, or TOAST
table

Good point.

Thanks, I'll see about committing this shortly.

Should I add this to the next commitfest (not an open item per se) or will
you be committing it?

What is happening with this?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 2017/06/13 0:29, Peter Eisentraut wrote:

On 4/24/17 21:22, Amit Langote wrote:

Hi Stephen,

On 2017/04/11 22:17, Stephen Frost wrote:

create extension pgrowlocks;
create view one as select 1;
select pgrowlocks('one');
-- ERROR: could not open file "base/68730/68748": No such file or directory

With the attached patch:

select pgrowlocks('one');
ERROR: "one" is not a table, index, materialized view, sequence, or TOAST
table

Good point.

Thanks, I'll see about committing this shortly.

Should I add this to the next commitfest (not an open item per se) or will
you be committing it?

What is happening with this?

FWIW, patch seems simple enough to be committed into 10, unless I am
missing something.

Rebased one attached.

Thanks,
Amit

On 6/12/17 21:10, Amit Langote wrote:

On 2017/06/13 0:29, Peter Eisentraut wrote:

On 4/24/17 21:22, Amit Langote wrote:

create extension pgrowlocks;
create view one as select 1;
select pgrowlocks('one');
-- ERROR: could not open file "base/68730/68748": No such file or directory

With the attached patch:

select pgrowlocks('one');
ERROR: "one" is not a table, index, materialized view, sequence, or TOAST
table

FWIW, patch seems simple enough to be committed into 10, unless I am
missing something.

Rebased one attached.

According to CheckValidRowMarkRel() in execMain.c, we don't allow row
locking in sequences, toast tables, and materialized views. This is not
quite the same as what your patch wants to do. I suppose we could still
allow reading the relation, and it won't ever show anything
interesting. What do you think?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 2017/06/13 22:53, Peter Eisentraut wrote:

On 6/12/17 21:10, Amit Langote wrote:

On 2017/06/13 0:29, Peter Eisentraut wrote:

On 4/24/17 21:22, Amit Langote wrote:

create extension pgrowlocks;
create view one as select 1;
select pgrowlocks('one');
-- ERROR: could not open file "base/68730/68748": No such file or directory

With the attached patch:

select pgrowlocks('one');
ERROR: "one" is not a table, index, materialized view, sequence, or TOAST
table

FWIW, patch seems simple enough to be committed into 10, unless I am
missing something.

Rebased one attached.

According to CheckValidRowMarkRel() in execMain.c, we don't allow row
locking in sequences, toast tables, and materialized views. This is not
quite the same as what your patch wants to do.

That's a good point. Perhaps, running pgrowlocks() should only be
supported for relation kinds that allow locking rows. That would be
logically consistent.

I suppose we could still
allow reading the relation, and it won't ever show anything
interesting. What do you think?

I was just thinking about fixing heap_beginscan() resulting in "could not
open file..." error (which is not very helpful) when pgrowlocks() is
passed the name of a file-less relation.

How about the attached patch that teaches pgrowlocks() to error out unless
a meaningful result can be returned? With the patch, it will issue a "%s
is not a table" message if it is not a RELKIND_RELATION, except a
different message will be issued for partitioned tables (for they are
tables). You might ask why I changed heap_openrv() to relation_openrv()?
That's because heap_openrv() results in "%s is an index" message if passed
an index relation, but perhaps it's better to be consistent about
outputting the same "%s is not a table" message even in all cases
including for indexes.

Thanks,
Amit

On 6/14/17 01:34, Amit Langote wrote:

How about the attached patch that teaches pgrowlocks() to error out unless
a meaningful result can be returned? With the patch, it will issue a "%s
is not a table" message if it is not a RELKIND_RELATION, except a
different message will be issued for partitioned tables (for they are
tables).

committed

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 2017/06/22 12:26, Peter Eisentraut wrote:

On 6/14/17 01:34, Amit Langote wrote:

How about the attached patch that teaches pgrowlocks() to error out unless
a meaningful result can be returned? With the patch, it will issue a "%s
is not a table" message if it is not a RELKIND_RELATION, except a
different message will be issued for partitioned tables (for they are
tables).

committed

Thanks.

Regards,
Amit

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