pgrowlocks relkind check

Started by Amit Langotealmost 9 years ago9 messages

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

1 attachment(s)

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

Attachments:

0001-Add-relkind-check-to-pgrowlocks.patchtext/x-diff; name=0001-Add-relkind-check-to-pgrowlocks.patchDownload

From c9cd61e7c78f3d8f8744a807399b22172055597b Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
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

Stephen Frost

sfrost@snowman.net

almost 9 years ago

In reply to: Amit Langote (#1)

Re: 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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Stephen Frost (#2)

Re: 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

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 8 years ago

In reply to: Amit Langote (#3)

Re: 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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Peter Eisentraut (#4)

1 attachment(s)

Re: 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

Attachments:

0001-Add-relkind-check-to-pgrowlocks.patchtext/plain; charset=UTF-8; name=0001-Add-relkind-check-to-pgrowlocks.patchDownload

From 570ae0006fccb9d4a2a53b93169e548050a12c07 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
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
 #define		Atnum_modes		4
 #define		Atnum_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

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 8 years ago

In reply to: Amit Langote (#5)

Re: 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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Peter Eisentraut (#6)

1 attachment(s)

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

Attachments:

0001-Teach-pgrowlocks-to-check-relkind-before-scanning.patchtext/plain; charset=UTF-8; name=0001-Teach-pgrowlocks-to-check-relkind-before-scanning.patchDownload

From 2db5f57f42c52236459ffefdd45cb3b7fe82ff56 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
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

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 8 years ago

In reply to: Amit Langote (#7)

Re: 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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Peter Eisentraut (#8)

Re: 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