Thread: [HACKERS] pgrowlocks relkind check
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
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
Attachment
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
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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 stillallow reading the relation, and it won't ever
showanything
interesting. What do you think?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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