Re: [HACKERS] pgrowlocks relkind check
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
Re: [HACKERS] pgrowlocks relkind check
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
Re: [HACKERS] 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
>From 2db5f57f42c52236459ffefdd45cb3b7fe82ff56 Mon Sep 17 00:00:00 2001
From: amit
Date: Wed, 14 Jun 2017 14:22:36 +0900
Subject: [PATCH] Teach pgrowlocks to check relkind before scanning
---
contrib/pgrowlocks/pgrowlocks.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index 00e2015c5c..5d52f9ec9a 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -97,7 +97,19 @@ pgrowlocks(PG_FUNCTION_ARGS)
relname = PG_GETARG_TEXT_PP(0);
relrv =
makeRangeVarFromNameList(textToQualifiedNameList(relname));
- rel = heap_openrv(relrv, AccessShareLock);
+ rel = relation_openrv(relrv, AccessShareLock);
+
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is a partitioned table",
+
RelationGetRelationName(rel)),
+errdetail("Partitioned tables do not
contain rows.")));
+ else if (rel->rd_rel->relkind != RELKIND_RELATION)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is not a table",
+
RelationGetRelationName(rel;
/*
* check permissions: must have SELECT on table or be in
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgrowlocks relkind check
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
Re: [HACKERS] pgrowlocks relkind check
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
>From 570ae0006fccb9d4a2a53b93169e548050a12c07 Mon Sep 17 00:00:00 2001
From: amit
Date: Tue, 11 Apr 2017 16:59:03 +0900
Subject: [PATCH] Add relkind check to pgrowlocks
---
contrib/pgrowlocks/pgrowlocks.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index 00e2015c5c..a1ee7f8034 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -66,6 +66,8 @@ typedef struct
#defineAtnum_modes 4
#defineAtnum_pids 5
+static void check_relation_relkind(Relation rel);
+
Datum
pgrowlocks(PG_FUNCTION_ARGS)
{
@@ -112,6 +114,9 @@ pgrowlocks(PG_FUNCTION_ARGS)
aclcheck_error(aclresult, ACL_KIND_CLASS,
RelationGetRelationName(rel));
+ /* Only some relkinds contain rows */
+ check_relation_relkind(rel);
+
scan = heap_beginscan(rel, GetActiveSnapshot(), 0, NULL);
mydata = palloc(sizeof(*mydata));
mydata->rel = rel;
@@ -298,3 +303,21 @@ pgrowlocks(PG_FUNCTION_ARGS)
SRF_RETURN_DONE(funcctx);
}
+
+/*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+ if (rel->rd_rel->relkind != RELKIND_RELATION &&
+ rel->rd_rel->relkind != RELKIND_INDEX &&
+ rel->rd_rel->relkind != RELKIND_MATVIEW &&
+ rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+ rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is not a table, index,
materialized view, sequence, or TOAST table",
+ RelationGetRelationName(rel;
+}
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgrowlocks relkind check
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
Re: [HACKERS] pgrowlocks relkind check
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
Re: [HACKERS] pgrowlocks relkind check
Amit,
* Amit Langote (langote_amit...@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
> 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
signature.asc
Description: Digital signature
[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
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
>From c9cd61e7c78f3d8f8744a807399b22172055597b Mon Sep 17 00:00:00 2001
From: amit
Date: Tue, 11 Apr 2017 16:59:03 +0900
Subject: [PATCH] Add relkind check to pgrowlocks
---
contrib/pgrowlocks/pgrowlocks.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index 8dd561c02a..900b034f1e 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -66,6 +66,8 @@ typedef struct
#define Atnum_modes 4
#define Atnum_pids 5
+static void check_relation_relkind(Relation rel);
+
Datum
pgrowlocks(PG_FUNCTION_ARGS)
{
@@ -109,6 +111,9 @@ pgrowlocks(PG_FUNCTION_ARGS)
aclcheck_error(aclresult, ACL_KIND_CLASS,
RelationGetRelationName(rel));
+ /* Only some relkinds contain rows */
+ check_relation_relkind(rel);
+
scan = heap_beginscan(rel, GetActiveSnapshot(), 0, NULL);
mydata = palloc(sizeof(*mydata));
mydata->rel = rel;
@@ -295,3 +300,21 @@ pgrowlocks(PG_FUNCTION_ARGS)
SRF_RETURN_DONE(funcctx);
}
+
+/*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+ if (rel->rd_rel->relkind != RELKIND_RELATION &&
+ rel->rd_rel->relkind != RELKIND_INDEX &&
+ rel->rd_rel->relkind != RELKIND_MATVIEW &&
+ rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+ rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+ ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is not a table, index, materialized view, sequence, or TOAST table",
+ RelationGetRelationName(rel;
+}
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
