Re: pgrowlocks relkind check

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