Skip to content

Commit ea97e44

Browse files
anarazelanarazel
committed
Harden tableam against nonexistant / wrong kind of AMs.
Previously it was allowed to set default_table_access_method to an empty string. That makes sense for default_tablespace, where that was copied from, as it signals falling back to the database's default tablespace. As there is no equivalent for table AMs, forbid that. Also make sure to throw a usable error when creating a table using an index AM, by using get_am_type_oid() to implement get_table_am_oid() instead of a separate copy. Previously we'd error out only later, in GetTableAmRoutine(). Thirdly remove GetTableAmRoutineByAmId() - it was only used in an earlier version of 8586bf7. Add tests for the above (some for index AMs as well).
1 parent 344b7e1 commit ea97e44

File tree

9 files changed

+119
-110
lines changed

9 files changed

+119
-110
lines changed

‎src/backend/access/table/tableamapi.c

Lines changed: 16 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,12 @@
1717
#include "access/xact.h"
1818
#include "catalog/pg_am.h"
1919
#include "catalog/pg_proc.h"
20+
#include "commands/defrem.h"
2021
#include "utils/fmgroids.h"
2122
#include "utils/memutils.h"
2223
#include "utils/syscache.h"
2324

2425

25-
static Oid get_table_am_oid(const char *tableamname, bool missing_ok);
26-
27-
2826
/*
2927
* GetTableAmRoutine
3028
* Call the specified access method handler routine to get its
@@ -41,7 +39,7 @@ GetTableAmRoutine(Oid amhandler)
4139
routine = (TableAmRoutine *) DatumGetPointer(datum);
4240

4341
if (routine == NULL || !IsA(routine, TableAmRoutine))
44-
elog(ERROR, "Table access method handler %u did not return a TableAmRoutine struct",
42+
elog(ERROR, "table access method handler %u did not return a TableAmRoutine struct",
4543
amhandler);
4644

4745
/*
@@ -98,106 +96,30 @@ GetTableAmRoutine(Oid amhandler)
9896
return routine;
9997
}
10098

101-
/*
102-
* GetTableAmRoutineByAmId - look up the handler of the table access
103-
* method with the given OID, and get its TableAmRoutine struct.
104-
*/
105-
const TableAmRoutine *
106-
GetTableAmRoutineByAmId(Oid amoid)
107-
{
108-
regproc amhandler;
109-
HeapTuple tuple;
110-
Form_pg_am amform;
111-
112-
/* Get handler function OID for the access method */
113-
tuple = SearchSysCache1(AMOID, ObjectIdGetDatum(amoid));
114-
if (!HeapTupleIsValid(tuple))
115-
elog(ERROR, "cache lookup failed for access method %u",
116-
amoid);
117-
amform = (Form_pg_am) GETSTRUCT(tuple);
118-
119-
/* Check that it is a table access method */
120-
if (amform->amtype != AMTYPE_TABLE)
121-
ereport(ERROR,
122-
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
123-
errmsg("access method \"%s\" is not of type %s",
124-
NameStr(amform->amname), "TABLE")));
125-
126-
amhandler = amform->amhandler;
127-
128-
/* Complain if handler OID is invalid */
129-
if (!RegProcedureIsValid(amhandler))
130-
ereport(ERROR,
131-
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
132-
errmsg("table access method \"%s\" does not have a handler",
133-
NameStr(amform->amname))));
134-
135-
ReleaseSysCache(tuple);
136-
137-
/* And finally, call the handler function to get the API struct. */
138-
return GetTableAmRoutine(amhandler);
139-
}
140-
141-
/*
142-
* get_table_am_oid - given a table access method name, look up the OID
143-
*
144-
* If missing_ok is false, throw an error if table access method name not
145-
* found. If true, just return InvalidOid.
146-
*/
147-
static Oid
148-
get_table_am_oid(const char *tableamname, bool missing_ok)
149-
{
150-
Oid result;
151-
Relation rel;
152-
TableScanDesc scandesc;
153-
HeapTuple tuple;
154-
ScanKeyData entry[1];
155-
156-
/*
157-
* Search pg_am. We use a heapscan here even though there is an index on
158-
* name, on the theory that pg_am will usually have just a few entries and
159-
* so an indexed lookup is a waste of effort.
160-
*/
161-
rel = heap_open(AccessMethodRelationId, AccessShareLock);
162-
163-
ScanKeyInit(&entry[0],
164-
Anum_pg_am_amname,
165-
BTEqualStrategyNumber, F_NAMEEQ,
166-
CStringGetDatum(tableamname));
167-
scandesc = table_beginscan_catalog(rel, 1, entry);
168-
tuple = heap_getnext(scandesc, ForwardScanDirection);
169-
170-
/* We assume that there can be at most one matching tuple */
171-
if (HeapTupleIsValid(tuple) &&
172-
((Form_pg_am) GETSTRUCT(tuple))->amtype == AMTYPE_TABLE)
173-
result = ((Form_pg_am) GETSTRUCT(tuple))->oid;
174-
else
175-
result = InvalidOid;
176-
177-
table_endscan(scandesc);
178-
heap_close(rel, AccessShareLock);
179-
180-
if (!OidIsValid(result) && !missing_ok)
181-
ereport(ERROR,
182-
(errcode(ERRCODE_UNDEFINED_OBJECT),
183-
errmsg("table access method \"%s\" does not exist",
184-
tableamname)));
185-
186-
return result;
187-
}
188-
18999
/* check_hook: validate new default_table_access_method */
190100
bool
191101
check_default_table_access_method(char **newval, void **extra, GucSource source)
192102
{
103+
if (**newval == '\0')
104+
{
105+
GUC_check_errdetail("default_table_access_method may not be empty.");
106+
return false;
107+
}
108+
109+
if (strlen(*newval) >= NAMEDATALEN)
110+
{
111+
GUC_check_errdetail("default_table_access_method is too long (maximum %d characters).",
112+
NAMEDATALEN - 1);
113+
return false;
114+
}
115+
193116
/*
194117
* If we aren't inside a transaction, we cannot do database access so
195118
* cannot verify the name. Must accept the value on faith.
196119
*/
197120
if (IsTransactionState())
198121
{
199-
if (**newval != '\0' &&
200-
!OidIsValid(get_table_am_oid(*newval, true)))
122+
if (!OidIsValid(get_table_am_oid(*newval, true)))
201123
{
202124
/*
203125
* When source == PGC_S_TEST, don't throw a hard error for a

‎src/backend/commands/amcmds.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,16 @@ get_index_am_oid(const char *amname, bool missing_ok)
189189
return get_am_type_oid(amname, AMTYPE_INDEX, missing_ok);
190190
}
191191

192+
/*
193+
* get_table_am_oid - given an access method name, look up its OID
194+
* and verify it corresponds to an table AM.
195+
*/
196+
Oid
197+
get_table_am_oid(const char *amname, bool missing_ok)
198+
{
199+
return get_am_type_oid(amname, AMTYPE_TABLE, missing_ok);
200+
}
201+
192202
/*
193203
* get_am_oid - given an access method name, look up its OID.
194204
* The type is not checked.

‎src/backend/commands/tablecmds.c

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -832,22 +832,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
832832
relkind == RELKIND_MATVIEW)
833833
accessMethod = default_table_access_method;
834834

835-
/*
836-
* look up the access method, verify it can handle the requested features
837-
*/
835+
/* look up the access method, verify it is for a table */
838836
if (accessMethod != NULL)
839-
{
840-
HeapTuple tuple;
841-
842-
tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethod));
843-
if (!HeapTupleIsValid(tuple))
844-
ereport(ERROR,
845-
(errcode(ERRCODE_UNDEFINED_OBJECT),
846-
errmsg("table access method \"%s\" does not exist",
847-
accessMethod)));
848-
accessMethodId = ((Form_pg_am) GETSTRUCT(tuple))->oid;
849-
ReleaseSysCache(tuple);
850-
}
837+
accessMethodId = get_table_am_oid(accessMethod, false);
851838

852839
/*
853840
* Create the relation. Inherited defaults and constraints are passed in

‎src/include/access/tableam.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1616,7 +1616,6 @@ extern void table_block_parallelscan_startblock_init(Relation rel,
16161616
*/
16171617

16181618
extern const TableAmRoutine *GetTableAmRoutine(Oid amhandler);
1619-
extern const TableAmRoutine *GetTableAmRoutineByAmId(Oid amoid);
16201619
extern const TableAmRoutine *GetHeapamTableAmRoutine(void);
16211620
extern bool check_default_table_access_method(char **newval, void **extra,
16221621
GucSource source);

‎src/include/commands/defrem.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ extern Datum transformGenericOptions(Oid catalogId,
156156
extern ObjectAddress CreateAccessMethod(CreateAmStmt *stmt);
157157
extern void RemoveAccessMethodById(Oid amOid);
158158
extern Oid get_index_am_oid(const char *amname, bool missing_ok);
159+
extern Oid get_table_am_oid(const char *amname, bool missing_ok);
159160
extern Oid get_am_oid(const char *amname, bool missing_ok);
160161
extern char *get_am_name(Oid amOid);
161162

‎src/test/regress/expected/create_am.out

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@
33
--
44
-- Make gist2 over gisthandler. In fact, it would be a synonym to gist.
55
CREATE ACCESS METHOD gist2 TYPE INDEX HANDLER gisthandler;
6+
-- Verify return type checks for handlers
7+
CREATE ACCESS METHOD bogus TYPE INDEX HANDLER int4in;
8+
ERROR: function int4in(internal) does not exist
9+
CREATE ACCESS METHOD bogus TYPE INDEX HANDLER heap_tableam_handler;
10+
ERROR: function heap_tableam_handler must return type index_am_handler
611
-- Try to create gist2 index on fast_emp4000: fail because opclass doesn't exist
712
CREATE INDEX grect2ind2 ON fast_emp4000 USING gist2 (home_base);
813
ERROR: data type box has no default operator class for access method "gist2"
@@ -102,8 +107,24 @@ NOTICE: drop cascades to index grect2ind2
102107
--
103108
-- Test table access methods
104109
--
110+
-- prevent empty values
111+
SET default_table_access_method = '';
112+
ERROR: invalid value for parameter "default_table_access_method": ""
113+
DETAIL: default_table_access_method may not be empty.
114+
-- prevent nonexistant values
115+
SET default_table_access_method = 'I do not exist AM';
116+
ERROR: invalid value for parameter "default_table_access_method": "I do not exist AM"
117+
DETAIL: Table access method "I do not exist AM" does not exist.
118+
-- prevent setting it to an index AM
119+
SET default_table_access_method = 'btree';
120+
ERROR: access method "btree" is not of type TABLE
105121
-- Create a heap2 table am handler with heapam handler
106122
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
123+
-- Verify return type checks for handlers
124+
CREATE ACCESS METHOD bogus TYPE TABLE HANDLER int4in;
125+
ERROR: function int4in(internal) does not exist
126+
CREATE ACCESS METHOD bogus TYPE TABLE HANDLER bthandler;
127+
ERROR: function bthandler must return type table_am_handler
107128
SELECT amname, amhandler, amtype FROM pg_am where amtype = 't' ORDER BY 1, 2;
108129
amname | amhandler | amtype
109130
--------+----------------------+--------
@@ -253,6 +274,18 @@ ORDER BY 3, 1, 2;
253274

254275
-- don't want to keep those tables, nor the default
255276
ROLLBACK;
277+
-- Third, check that we can neither create a table using a nonexistant
278+
-- AM, nor using an index AM
279+
CREATE TABLE i_am_a_failure() USING "";
280+
ERROR: zero-length delimited identifier at or near """"
281+
LINE 1: CREATE TABLE i_am_a_failure() USING "";
282+
^
283+
CREATE TABLE i_am_a_failure() USING i_do_not_exist_am;
284+
ERROR: access method "i_do_not_exist_am" does not exist
285+
CREATE TABLE i_am_a_failure() USING "I do not exist AM";
286+
ERROR: access method "I do not exist AM" does not exist
287+
CREATE TABLE i_am_a_failure() USING "btree";
288+
ERROR: access method "btree" is not of type TABLE
256289
-- Drop table access method, which fails as objects depends on it
257290
DROP ACCESS METHOD heap2;
258291
ERROR: cannot drop access method heap2 because other objects depend on it

‎src/test/regress/expected/type_sanity.out

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -520,6 +520,24 @@ WHERE p1.relkind IN ('S', 'v', 'f', 'c') and
520520
-----+---------
521521
(0 rows)
522522

523+
-- Indexes should have AMs of type 'i'
524+
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
525+
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
526+
WHERE pc.relkind IN ('i') and
527+
pa.amtype != 'i';
528+
oid | relname | amname | amtype
529+
-----+---------+--------+--------
530+
(0 rows)
531+
532+
-- Tables, matviews etc should have AMs of type 't'
533+
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
534+
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
535+
WHERE pc.relkind IN ('r', 't', 'm') and
536+
pa.amtype != 't';
537+
oid | relname | amname | amtype
538+
-----+---------+--------+--------
539+
(0 rows)
540+
523541
-- **************** pg_attribute ****************
524542
-- Look for illegal values in pg_attribute fields
525543
SELECT p1.attrelid, p1.attname

‎src/test/regress/sql/create_am.sql

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@
55
-- Make gist2 over gisthandler. In fact, it would be a synonym to gist.
66
CREATE ACCESS METHOD gist2 TYPE INDEX HANDLER gisthandler;
77

8+
-- Verify return type checks for handlers
9+
CREATE ACCESS METHOD bogus TYPE INDEX HANDLER int4in;
10+
CREATE ACCESS METHOD bogus TYPE INDEX HANDLER heap_tableam_handler;
11+
12+
813
-- Try to create gist2 index on fast_emp4000: fail because opclass doesn't exist
914
CREATE INDEX grect2ind2 ON fast_emp4000 USING gist2 (home_base);
1015

@@ -72,11 +77,26 @@ DROP ACCESS METHOD gist2 CASCADE;
7277
-- Test table access methods
7378
--
7479

80+
-- prevent empty values
81+
SET default_table_access_method = '';
82+
83+
-- prevent nonexistant values
84+
SET default_table_access_method = 'I do not exist AM';
85+
86+
-- prevent setting it to an index AM
87+
SET default_table_access_method = 'btree';
88+
89+
7590
-- Create a heap2 table am handler with heapam handler
7691
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
7792

93+
-- Verify return type checks for handlers
94+
CREATE ACCESS METHOD bogus TYPE TABLE HANDLER int4in;
95+
CREATE ACCESS METHOD bogus TYPE TABLE HANDLER bthandler;
96+
7897
SELECT amname, amhandler, amtype FROM pg_am where amtype = 't' ORDER BY 1, 2;
7998

99+
80100
-- First create tables employing the new AM using USING
81101

82102
-- plain CREATE TABLE
@@ -178,6 +198,13 @@ ORDER BY 3, 1, 2;
178198
-- don't want to keep those tables, nor the default
179199
ROLLBACK;
180200

201+
-- Third, check that we can neither create a table using a nonexistant
202+
-- AM, nor using an index AM
203+
CREATE TABLE i_am_a_failure() USING "";
204+
CREATE TABLE i_am_a_failure() USING i_do_not_exist_am;
205+
CREATE TABLE i_am_a_failure() USING "I do not exist AM";
206+
CREATE TABLE i_am_a_failure() USING "btree";
207+
181208
-- Drop table access method, which fails as objects depends on it
182209
DROP ACCESS METHOD heap2;
183210

‎src/test/regress/sql/type_sanity.sql

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,18 @@ FROM pg_class as p1
379379
WHERE p1.relkind IN ('S', 'v', 'f', 'c') and
380380
p1.relam != 0;
381381

382+
-- Indexes should have AMs of type 'i'
383+
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
384+
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
385+
WHERE pc.relkind IN ('i') and
386+
pa.amtype != 'i';
387+
388+
-- Tables, matviews etc should have AMs of type 't'
389+
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
390+
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
391+
WHERE pc.relkind IN ('r', 't', 'm') and
392+
pa.amtype != 't';
393+
382394
-- **************** pg_attribute ****************
383395

384396
-- Look for illegal values in pg_attribute fields

0 commit comments

Comments
 (0)