Re: pgpool versus sequences - Mailing list pgsql-hackers

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

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 620efda838421a94a9835a7e8db1d8fa4b50e1ea..2d491fe6c876e34bd757ab271fada1f959707447 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 == 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 = (ExecRowMark *) palloc(sizeof(ExecRowMark));
          erm->relation = relation;
--- 838,846 ----
                  break;
          }

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

          erm = (ExecRowMark *) palloc(sizeof(ExecRowMark));
          erm->relation = 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 trigger.
+  *
+  * 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 checking