10BC0 Clean up handling of XactReadOnly and RecoveryInProgress checks. · postgres/postgres@05d8a56 · GitHub
[go: up one dir, main page]
Skip to content

Commit 05d8a56

Browse files
tglsfdctglsfdc
committed
Clean up handling of XactReadOnly and RecoveryInProgress checks.
Add some checks that seem logically necessary, in particular let's make real sure that HS slave sessions cannot create temp tables. (If they did they would think that temp tables belonging to the master's session with the same BackendId were theirs. We *must* not allow myTempNamespace to become set in a slave session.) Change setval() and nextval() so that they are only allowed on temp sequences in a read-only transaction. This seems consistent with what we allow for table modifications in read-only transactions. Since an HS slave can't have a temp sequence, this also provides a nicer cure for the setval PANIC reported by Erik Rijkers. Make the error messages more uniform, and have them mention the specific command being complained of. This seems worth the trifling amount of extra code, since people are likely to see such messages a lot more than before.
1 parent fada420 commit 05d8a56

File tree

12 files changed

+114
-64
lines changed

12 files changed

+114
-64
lines changed

‎src/backend/access/transam/varsup.c‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Copyright (c) 2000-2010, PostgreSQL Global Development Group
77
*
88
* IDENTIFICATION
9-
* $PostgreSQL: pgsql/src/backend/access/transam/varsup.c,v 1.89 2010/02/17 03:10:33 tgl Exp $
9+
* $PostgreSQL: pgsql/src/backend/access/transam/varsup.c,v 1.90 2010/02/20 21:24:01 tgl Exp $
1010
*
1111
*-------------------------------------------------------------------------
1212
*/
@@ -58,6 +58,10 @@ GetNewTransactionId(bool isSubXact)
5858
return BootstrapTransactionId;
5959
}
6060

61+
/* safety check, we should never get this far in a HS slave */
62+
if (RecoveryInProgress())
63+
elog(ERROR, "cannot assign TransactionIds during recovery");
64+
6165
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
6266

6367
xid = ShmemVariableCache->nextXid;
@@ -420,6 +424,10 @@ GetNewObjectId(void)
420424
{
421425
Oid result;
422426

427+
/* safety check, we should never get this far in a HS slave */
428+
if (RecoveryInProgress())
429+
elog(ERROR, "cannot assign OIDs during recovery");
430+
423431
LWLockAcquire(OidGenLock, LW_EXCLUSIVE);
424432

425433
/*

‎src/backend/access/transam/xact.c‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*
1111
*
1212
* IDENTIFICATION
13-
* $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.287 2010/02/17 04:19:39 tgl Exp $
13+
* $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.288 2010/02/20 21:24:01 tgl Exp $
1414
*
1515
*-------------------------------------------------------------------------
1616
*/
@@ -402,9 +402,6 @@ AssignTransactionId(TransactionState s)
402402
bool isSubXact = (s->parent != NULL);
403403
ResourceOwner currentOwner;
404404

405-
if (RecoveryInProgress())
406-
elog(ERROR, "cannot assign TransactionIds during recovery");
407-
408405
/* Assert that caller didn't screw up */
409406
Assert(!TransactionIdIsValid(s->transactionId));
410407
Assert(s->state == TRANS_INPROGRESS);

‎src/backend/catalog/namespace.c‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* Portions Copyright (c) 1994, Regents of the University of California
1414
*
1515
* IDENTIFICATION
16-
* $PostgreSQL: pgsql/src/backend/catalog/namespace.c,v 1.123 2010/02/14 18:42:13 rhaas Exp $
16+
* $PostgreSQL: pgsql/src/backend/catalog/namespace.c,v 1.124 2010/02/20 21:24:01 tgl Exp $
1717
*
1818
*-------------------------------------------------------------------------
1919
*/
@@ -3017,6 +3017,21 @@ InitTempTableNamespace(void)
30173017
errmsg("permission denied to create temporary tables in database \"%s\"",
30183018
get_database_name(MyDatabaseId))));
30193019

3020+
/*
3021+
* Do not allow a Hot Standby slave session to make temp tables. Aside
3022+
* from problems with modifying the system catalogs, there is a naming
3023+
* conflict: pg_temp_N belongs to the session with BackendId N on the
3024+
* master, not to a slave session with the same BackendId. We should
3025+
* not be able to get here anyway due to XactReadOnly checks, but let's
3026+
* just make real sure. Note that this also backstops various operations
3027+
* that allow XactReadOnly transactions to modify temp tables; they'd need
3028+
* RecoveryInProgress checks if not for this.
3029+
*/
3030+
if (RecoveryInProgress())
3031+
ereport(ERROR,
3032+
(errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
3033+
errmsg("cannot create temporary tables during recovery")));
3034+
30203035
snprintf(namespaceName, sizeof(namespaceName), "pg_temp_%d", MyBackendId);
30213036

30223037
namespaceId = GetSysCacheOid1(NAMESPACENAME,

‎src/backend/commands/async.c‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
99
* IDENTIFICATION
10-
* $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.153 2010/02/17 16:54:06 tgl Exp $
10+
* $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.154 2010/02/20 21:24:02 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -534,6 +534,9 @@ pg_notify(PG_FUNCTION_ARGS)
534534
else
535535
payload = text_to_cstring(PG_GETARG_TEXT_PP(1));
536536

537+
/* For NOTIFY as a statement, this is checked in ProcessUtility */
538+
PreventCommandDuringRecovery("NOTIFY");
539+
537540
Async_Notify(channel, payload);
538541

539542
PG_RETURN_VOID();

‎src/backend/commands/copy.c‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/copy.c,v 1.324 2010/02/08 04:33:53 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/copy.c,v 1.325 2010/02/20 21:24:02 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -1023,9 +1023,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString)
10231023

10241024
/* check read-only transaction */
10251025
if (XactReadOnly && is_from && !cstate->rel->rd_islocaltemp)
1026-
ereport(ERROR,
1027-
(errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
1028-
errmsg("transaction is read-only")));
1026+
PreventCommandIfReadOnly("COPY FROM");
10291027

10301028
/* Don't allow COPY w/ OIDs to or from a table without them */
10311029
if (cstate->oids && !cstate->rel->rd_rel->relhasoids)

‎src/backend/commands/lockcmds.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.27 2010/01/02 16:57:37 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.28 2010/02/20 21:24:02 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -55,7 +55,7 @@ LockTableCommand(LockStmt *lockstmt)
5555
* This test must match the restrictions defined in LockAcquire()
5656
*/
5757
if (lockstmt->mode > RowExclusiveLock)
58-
PreventCommandDuringRecovery();
58+
PreventCommandDuringRecovery("LOCK TABLE");
5959

6060
LockTableRecurse(reloid, relation,
6161
lockstmt->mode, lockstmt->nowait, recurse);

‎src/backend/commands/sequence.c‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/sequence.c,v 1.167 2010/02/19 06:29:19 heikki Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/sequence.c,v 1.168 2010/02/20 21:24:02 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -459,9 +459,6 @@ nextval_internal(Oid relid)
459459
rescnt = 0;
460460
bool logit = false;
461461

462-
/* nextval() writes to database and must be prevented during recovery */
463-
PreventCommandDuringRecovery();
464-
465462
/* open and AccessShareLock sequence */
466463
init_sequence(relid, &elm, &seqrel);
467464

@@ -472,6 +469,10 @@ nextval_internal(Oid relid)
472469
errmsg("permission denied for sequence %s",
473470
RelationGetRelationName(seqrel))));
474471

472+
/* read-only transactions may only modify temp sequences */
473+
if (!seqrel->rd_islocaltemp)
474+
PreventCommandIfReadOnly("nextval()");
475+
475476
if (elm->last != elm->cached) /* some numbers were cached */
476477
{
477478
Assert(elm->last_valid);
@@ -736,9 +737,6 @@ do_setval(Oid relid, int64 next, bool iscalled)
736737
Buffer buf;
737738
Form_pg_sequence seq;
738739

739-
/* setval() writes to database and must be prevented during recovery */
740-
PreventCommandDuringRecovery();
741-
742740
/* open and AccessShareLock sequence */
743741
init_sequence(relid, &elm, &seqrel);
744742

@@ -748,6 +746,10 @@ do_setval(Oid relid, int64 next, bool iscalled)
748746
errmsg("permission denied for sequence %s",
749747
RelationGetRelationName(seqrel))));
750748

749+
/* read-only transactions may only modify temp sequences */
750+
if (!seqrel->rd_islocaltemp)
751+
PreventCommandIfReadOnly("setval()");
752+
751753
/* lock page' buffer and read tuple */
752754
seq = read_info(elm, seqrel, &buf);
753755

‎src/backend/executor/execMain.c‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
*
2727
*
2828
* IDENTIFICATION
29-
* $PostgreSQL: pgsql/src/backend/exec 3044 utor/execMain.c,v 1.346 2010/02/09 21:43:30 tgl Exp $
29+
* $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.347 2010/02/20 21:24:02 tgl Exp $
3030
*
3131
*-------------------------------------------------------------------------
3232
*/
@@ -50,6 +50,7 @@
5050
#include "storage/bufmgr.h"
5151
#include "storage/lmgr.h"
5252
#include "storage/smgr.h"
53+
#include "tcop/utility.h"
5354
#include "utils/acl.h"
5455
#include "utils/lsyscache.h"
5556
#include "utils/memutils.h"
@@ -568,6 +569,10 @@ ExecCheckRTEPerms(RangeTblEntry *rte)
568569

569570
/*
570571
* Check that the query does not imply any writes to non-temp tables.
572+
*
573+
* Note: in a Hot Standby slave this would need to reject writes to temp
574+
* tables as well; but an HS slave can't have created any temp tables
575+
* in the first place, so no need to check that.
571576
*/
572577
static void
573578
ExecCheckXactReadOnly(PlannedStmt *plannedstmt)
@@ -577,10 +582,11 @@ ExecCheckXactReadOnly(PlannedStmt *plannedstmt)
577582
/*
578583
* CREATE TABLE AS or SELECT INTO?
579584
*
580-
* XXX should we allow this if the destination is temp?
585+
* XXX should we allow this if the destination is temp? Considering
586+
* that it would still require catalog changes, probably not.
581587
*/
582588
if (plannedstmt->intoClause != NULL)
583-
goto fail;
589+
PreventCommandIfReadOnly(CreateCommandTag((Node *) plannedstmt));
584590

585591
/* Fail if write permissions are requested on any non-temp table */
586592
foreach(l, plannedstmt->rtable)
@@ -596,15 +602,8 @@ ExecCheckXactReadOnly(PlannedStmt *plannedstmt)
596602
if (isTempNamespace(get_rel_namespace(rte->relid)))
597603
continue;
598604

599-
goto fail;
605+
PreventCommandIfReadOnly(CreateCommandTag((Node *) plannedstmt));
600606
}
601-
602-
return;
603-
604-
fail:
605-
ereport(ERROR,
606-
(errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
607-
errmsg("transaction is read-only")));
608607
}
609608

610609

0 commit comments

Comments
 (0)
0