To: Tatsuo Ishii <ishii@postgresql.org>
cc: mangoo@wpkg.org, scott.marlowe@gmail.com, t-ishii@sra.co.jp,
	Kevin.Grittner@wicourts.gov, pgsql-hackers@postgresql.org
Subject: Re: pgpool versus sequences 
In-reply-to: <20110601.001430.1098605953350881110.t-ishii@sraoss.co.jp> 
References: <16063.1306811813@sss.pgh.pa.us> <4DE4A652.2040301@wpkg.org>
	<25524.1306848814@sss.pgh.pa.us>
	<20110601.001430.1098605953350881110.t-ishii@sraoss.co.jp>
Comments: In-reply-to Tatsuo Ishii <ishii@postgresql.org>
	message dated "Wed, 01 Jun 2011 00:14:30 +0900"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <26885.1306965838.0@sss.pgh.pa.us>
Date: Wed, 01 Jun 2011 18:04:58 -0400
Message-ID: <26909.1306965898@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <26885.1306965838.1@sss.pgh.pa.us>

I wrote:
>>>> I think the most appropriate solution may be to disallow SELECT FOR
>>>> UPDATE/SHARE on sequences ... so if you have a good reason why we
>>>> shouldn't do so, please explain it.

Attached is a proposed patch to close off this hole.  I found that
somebody had already inserted code to forbid the case for foreign
tables, so I just extended that idea a bit (by copying-and-pasting
CheckValidResultRel).  Questions:

* Does anyone want to bikeshed on the wording of the error messages?
* Does anyone want to argue for not forbidding SELECT FOR UPDATE on
  toast tables?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-patch; charset="us-ascii"
Content-ID: <26885.1306965838.2@sss.pgh.pa.us>
Content-Description: disallow-row-locks-on-non-tables.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMa=
in.c
index 620efda838421a94a9835a7e8db1d8fa4b50e1ea..2d491fe6c876e34bd757ab271f=
ada1f959707447 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** ExecutorCheckPerms_hook_type ExecutorChe
*** 74,79 ****
--- 74,80 ----
  =

  /* decls for local routines only used within this module */
  static void InitPlan(QueryDesc *queryDesc, int eflags);
+ static void CheckValidMarkRel(Relation rel, RowMarkType markType);
  static void ExecPostprocessPlan(EState *estate);
  static void ExecEndPlan(PlanState *planstate, EState *estate);
  static void ExecutePlan(EState *estate, PlanState *planstate,
*************** InitPlan(QueryDesc *queryDesc, int eflag
*** 837,848 ****
  				break;
  		}
  =

! 		/* if foreign table, tuples can't be locked */
! 		if (relation && relation->rd_rel->relkind =3D=3D RELKIND_FOREIGN_TABLE=
)
! 			ereport(ERROR,
! 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 					 errmsg("SELECT FOR UPDATE/SHARE cannot be used with foreign table =
\"%s\"",
! 							RelationGetRelationName(relation))));
  =

  		erm =3D (ExecRowMark *) palloc(sizeof(ExecRowMark));
  		erm->relation =3D relation;
--- 838,846 ----
  				break;
  		}
  =

! 		/* Check that relation is a legal target for marking */
! 		if (relation)
! 			CheckValidMarkRel(relation, rc->markType);
  =

  		erm =3D (ExecRowMark *) palloc(sizeof(ExecRowMark));
  		erm->relation =3D relation;
*************** InitPlan(QueryDesc *queryDesc, int eflag
*** 977,982 ****
--- 975,982 ----
   * In most cases parser and/or planner should have noticed this already,=
 but
   * let's make sure.  In the view case we do need a test here, because if=
 the
   * view wasn't rewritten by a rule, it had better have an INSTEAD trigge=
r.
+  *
+  * Note: when changing this function, see also CheckValidMarkRel.
   */
  void
  CheckValidResultRel(Relation resultRel, CmdType operation)
*************** CheckValidResultRel(Relation resultRel, =

*** 1048,1053 ****
--- 1048,1104 ----
  }
  =

  /*
+  * Check that a proposed rowmark target relation is a legal target
+  *
+  * In most cases parser and/or planner should have noticed this already,=
 but
+  * they don't cover all cases.
+  */
+ static void
+ CheckValidMarkRel(Relation rel, RowMarkType markType)
+ {
+ 	switch (rel->rd_rel->relkind)
+ 	{
+ 		case RELKIND_RELATION:
+ 			/* OK */
+ 			break;
+ 		case RELKIND_SEQUENCE:
+ 			/* Must disallow this because we don't vacuum sequences */
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ 					 errmsg("cannot lock rows in sequence \"%s\"",
+ 							RelationGetRelationName(rel))));
+ 			break;
+ 		case RELKIND_TOASTVALUE:
+ 			/* We could allow this, but there seems no good reason to */
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ 					 errmsg("cannot lock rows in TOAST relation \"%s\"",
+ 							RelationGetRelationName(rel))));
+ 			break;
+ 		case RELKIND_VIEW:
+ 			/* Should not get here */
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ 					 errmsg("cannot lock rows in view \"%s\"",
+ 							RelationGetRelationName(rel))));
+ 			break;
+ 		case RELKIND_FOREIGN_TABLE:
+ 			/* Perhaps we can support this someday, but not today */
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ 					 errmsg("cannot lock rows in foreign table \"%s\"",
+ 							RelationGetRelationName(rel))));
+ 			break;
+ 		default:
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ 					 errmsg("cannot lock rows in relation \"%s\"",
+ 							RelationGetRelationName(rel))));
+ 			break;
+ 	}
+ }
+ =

+ /*
   * Initialize ResultRelInfo data for one result relation
   *
   * Caution: before Postgres 9.1, this function included the relkind chec=
king

------- =_aaaaaaaaaa0--