On Wed, Apr 08, 2020 at 04:13:31PM +0900, Michael Paquier wrote: > I have been looking at the tree and the use of the table AM APIs, and > those TID lookups are really a particular case compared to the other > callers of the table AM callbacks. Anyway, I have not spotted similar > problems, so I find very tempting the option to just add some > RELKIND_HAS_STORAGE() to tid.c where it matters and call it a day.
Playing more with this stuff, it happens that we have zero code coverage for currtid() and currtid2(), and the main user of those functions I can find around is the ODBC driver: https://coverage.postgresql.org/src/backend/utils/adt/tid.c.gcov.html There are multiple cases to consider, particularly for views: - Case of a view with ctid as attribute taken from table. - Case of a view with ctid as attribute with incorrect attribute type. It is worth noting that all those code paths can trigger various elog() errors, which is not something that a user should be able to do using a SQL-callable function. There are also two code paths for cases where a view has no or more-than-one SELECT rules, which cannot normally be reached. All in that, I propose something like the attached to patch the surroundings with tests to cover everything I could think of, which I guess had better be backpatched? While on it, I have noticed that we lack coverage for max(tid) and min(tid), so I have included a bonus test. Another issue is that we don't have any documentation for those functions, in which case the best fit is a subsection for TID operators under "Functions and Operators"? For now, I am adding a patch to next CF so as we don't forget about this set of issues. Any thoughts? -- Michael
diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c
index 4ce8375eab..90e03e890d 100644
--- a/src/backend/utils/adt/tid.c
+++ b/src/backend/utils/adt/tid.c
@@ -31,6 +31,7 @@
#include "parser/parsetree.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
#include "utils/varlena.h"
@@ -300,20 +301,41 @@ currtid_for_view(Relation viewrel, ItemPointer tid)
for (i = 0; i < natts; i++)
{
Form_pg_attribute attr = TupleDescAttr(att, i);
+ char *attname = NameStr(attr->attname);
- if (strcmp(NameStr(attr->attname), "ctid") == 0)
+ if (strcmp(attname, "ctid") == 0)
{
if (attr->atttypid != TIDOID)
- elog(ERROR, "ctid isn't of type TID");
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("cannot use attribute \"%s\" of type %s on view \"%s.%s\"",
+ attname, format_type_be(attr->atttypid),
+ get_namespace_name(RelationGetNamespace(viewrel)),
+ RelationGetRelationName(viewrel))));
tididx = i;
break;
}
}
+
if (tididx < 0)
- elog(ERROR, "currtid cannot handle views with no CTID");
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot look at latest visible tid for (%u, %u) on view \"%s.%s\" without CTID",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ get_namespace_name(RelationGetNamespace(viewrel)),
+ RelationGetRelationName(viewrel))));
+
rulelock = viewrel->rd_rules;
+
if (!rulelock)
- elog(ERROR, "the view has no rules");
+ elog(ERROR,
+ "cannot look at latest visible tid for (%u, %u) on view \"%s.%s\" with no rules",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ get_namespace_name(RelationGetNamespace(viewrel)),
+ RelationGetRelationName(viewrel));
+
for (i = 0; i < rulelock->numLocks; i++)
{
rewrite = rulelock->rules[i];
@@ -323,7 +345,13 @@ currtid_for_view(Relation viewrel, ItemPointer tid)
TargetEntry *tle;
if (list_length(rewrite->actions) != 1)
- elog(ERROR, "only one select rule is allowed in views");
+ elog(ERROR,
+ "cannot look at latest visible tid for (%u, %u) on view \"%s.%s\" with multiple SELECT rules",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ get_namespace_name(RelationGetNamespace(viewrel)),
+ RelationGetRelationName(viewrel));
+
query = (Query *) linitial(rewrite->actions);
tle = get_tle_by_resno(query->targetList, tididx + 1);
if (tle && tle->expr && IsA(tle->expr, Var))
@@ -345,10 +373,21 @@ currtid_for_view(Relation viewrel, ItemPointer tid)
break;
}
}
- elog(ERROR, "currtid cannot handle this view");
+
+ elog(ERROR, "could not look at latest visible tid for (%u, %u) on view \"%s.%s\"",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ get_namespace_name(RelationGetNamespace(viewrel)),
+ RelationGetRelationName(viewrel));
return (Datum) 0;
}
+/*
+ * currtid_byreloid
+ * Return the latest visible tid of a relation's tuple, associated
+ * to the tid given in input. This is a wrapper for currtid(), and
+ * uses in input the OID of the relation to look at.
+ */
Datum
currtid_byreloid(PG_FUNCTION_ARGS)
{
@@ -378,6 +417,15 @@ currtid_byreloid(PG_FUNCTION_ARGS)
if (rel->rd_rel->relkind == RELKIND_VIEW)
return currtid_for_view(rel, tid);
+ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot look at latest visible tid for (%u, %u) on relation \"%s.%s\"",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ get_namespace_name(RelationGetNamespace(rel)),
+ RelationGetRelationName(rel))));
+
ItemPointerCopy(tid, result);
snapshot = RegisterSnapshot(GetLatestSnapshot());
@@ -391,6 +439,12 @@ currtid_byreloid(PG_FUNCTION_ARGS)
PG_RETURN_ITEMPOINTER(result);
}
+/*
+ * currtid_byrename
+ * Return the latest visible tid of a relation's tuple, associated
+ * to the tid given in input. This is a wrapper for currtid2(), and
+ * uses in input the relation name to look at.
+ */
Datum
currtid_byrelname(PG_FUNCTION_ARGS)
{
@@ -415,6 +469,15 @@ currtid_byrelname(PG_FUNCTION_ARGS)
if (rel->rd_rel->relkind == RELKIND_VIEW)
return currtid_for_view(rel, tid);
+ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot look at latest visible tid for (%u, %u) on relation \"%s.%s\"",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ get_namespace_name(RelationGetNamespace(rel)),
+ RelationGetRelationName(rel))));
+
result = (ItemPointer) palloc(sizeof(ItemPointerData));
ItemPointerCopy(tid, result);
diff --git a/src/test/regress/expected/tid.out b/src/test/regress/expected/tid.out
new file mode 100644
index 0000000000..9862830bce
--- /dev/null
+++ b/src/test/regress/expected/tid.out
@@ -0,0 +1,106 @@
+-- tests for functions related to TID handling
+CREATE TABLE tid_tab (a int);
+-- min() and max() for TIDs
+INSERT INTO tid_tab VALUES (1), (2);
+SELECT min(ctid) FROM tid_tab;
+ min
+-------
+ (0,1)
+(1 row)
+
+SELECT max(ctid) FROM tid_tab;
+ max
+-------
+ (0,2)
+(1 row)
+
+TRUNCATE tid_tab;
+-- Tests for currtid() and currtid2() with various relation kinds
+-- Materialized view
+CREATE MATERIALIZED VIEW tid_matview AS SELECT a FROM tid_tab;
+SELECT currtid('tid_matview'::regclass::oid, '(0,1)'::tid); -- fails
+ERROR: tid (0, 1) is not valid for relation "tid_matview"
+SELECT currtid2('tid_matview'::text, '(0,1)'::tid); -- fails
+ERROR: tid (0, 1) is not valid for relation "tid_matview"
+INSERT INTO tid_tab VALUES (1);
+REFRESH MATERIALIZED VIEW tid_matview;
+SELECT currtid('tid_matview'::regclass::oid, '(0,1)'::tid); -- ok
+ currtid
+---------
+ (0,1)
+(1 row)
+
+SELECT currtid2('tid_matview'::text, '(0,1)'::tid); -- ok
+ currtid2
+----------
+ (0,1)
+(1 row)
+
+DROP MATERIALIZED VIEW tid_matview;
+TRUNCATE tid_tab;
+-- Sequence
+CREATE SEQUENCE tid_seq;
+SELECT currtid('tid_seq'::regclass::oid, '(0,1)'::tid); -- ok
+ currtid
+---------
+ (0,1)
+(1 row)
+
+SELECT currtid2('tid_seq'::text, '(0,1)'::tid); -- ok
+ currtid2
+----------
+ (0,1)
+(1 row)
+
+DROP SEQUENCE tid_seq;
+-- Index, fails with incorrect relation type
+CREATE INDEX tid_ind ON tid_tab(a);
+SELECT currtid('tid_ind'::regclass::oid, '(0,1)'::tid); -- fails
+ERROR: "tid_ind" is an index
+SELECT currtid2('tid_ind'::text, '(0,1)'::tid); -- fails
+ERROR: "tid_ind" is an index
+DROP INDEX tid_ind;
+-- Partitioned table, no storage
+CREATE TABLE tid_part (a int) PARTITION BY RANGE (a);
+SELECT currtid('tid_part'::regclass::oid, '(0,1)'::tid); -- fails
+ERROR: cannot look at latest visible tid for (0, 1) on relation "public.tid_part"
+SELECT currtid2('tid_part'::text, '(0,1)'::tid); -- fails
+ERROR: cannot look at latest visible tid for (0, 1) on relation "public.tid_part"
+DROP TABLE tid_part;
+-- Views
+-- ctid not defined in the view
+CREATE VIEW tid_view_no_ctid AS SELECT a FROM tid_tab;
+SELECT currtid('tid_view_no_ctid'::regclass::oid, '(0,1)'::tid); -- fails
+ERROR: cannot look at latest visible tid for (0, 1) on view "public.tid_view_no_ctid" without CTID
+SELECT currtid2('tid_view_no_ctid'::text, '(0,1)'::tid); -- fails
+ERROR: cannot look at latest visible tid for (0, 1) on view "public.tid_view_no_ctid" without CTID
+DROP VIEW tid_view_no_ctid;
+-- ctid fetched directly from the source table.
+CREATE VIEW tid_view_with_ctid AS SELECT ctid, a FROM tid_tab;
+SELECT currtid('tid_view_with_ctid'::regclass::oid, '(0,1)'::tid); -- fails
+ERROR: tid (0, 1) is not valid for relation "tid_tab"
+SELECT currtid2('tid_view_with_ctid'::text, '(0,1)'::tid); -- fails
+ERROR: tid (0, 1) is not valid for relation "tid_tab"
+INSERT INTO tid_tab VALUES (1);
+SELECT currtid('tid_view_with_ctid'::regclass::oid, '(0,1)'::tid); -- ok
+ currtid
+---------
+ (0,1)
+(1 row)
+
+SELECT currtid2('tid_view_with_ctid'::text, '(0,1)'::tid); -- ok
+ currtid2
+----------
+ (0,1)
+(1 row)
+
+DROP VIEW tid_view_with_ctid;
+TRUNCATE tid_tab;
+-- ctid attribute with incorrect data type
+CREATE VIEW tid_view_fake_ctid AS SELECT 1 AS ctid, 2 AS a;
+SELECT currtid('tid_view_fake_ctid'::regclass::oid, '(0,1)'::tid); -- fails
+ERROR: cannot use attribute "ctid" of type integer on view "public.tid_view_fake_ctid"
+SELECT currtid2('tid_view_fake_ctid'::text, '(0,1)'::tid); -- fails
+ERROR: cannot use attribute "ctid" of type integer on view "public.tid_view_fake_ctid"
+DROP VIEW tid_view_fake_ctid;
+DROP TABLE tid_tab CASCADE;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 95f1925072..026ea880cd 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -78,7 +78,7 @@ test: brin gin gist spgist privileges init_privs security_label collate matview
# ----------
# Another group of parallel tests
# ----------
-test: create_table_like alter_generic alter_operator misc async dbsize misc_functions sysviews tsrf tidscan collate.icu.utf8 incremental_sort
+test: create_table_like alter_generic alter_operator misc async dbsize misc_functions sysviews tsrf tid tidscan collate.icu.utf8 incremental_sort
# rules cannot run concurrently with any test that creates
# a view or rule in the public schema
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 8ba4136220..979d926119 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -135,6 +135,7 @@ test: dbsize
test: misc_functions
test: sysviews
test: tsrf
+test: tid
test: tidscan
test: collate.icu.utf8
test: rules
diff --git a/src/test/regress/sql/tid.sql b/src/test/regress/sql/tid.sql
new file mode 100644
index 0000000000..c0d02df34f
--- /dev/null
+++ b/src/test/regress/sql/tid.sql
@@ -0,0 +1,63 @@
+-- tests for functions related to TID handling
+
+CREATE TABLE tid_tab (a int);
+
+-- min() and max() for TIDs
+INSERT INTO tid_tab VALUES (1), (2);
+SELECT min(ctid) FROM tid_tab;
+SELECT max(ctid) FROM tid_tab;
+TRUNCATE tid_tab;
+
+-- Tests for currtid() and currtid2() with various relation kinds
+
+-- Materialized view
+CREATE MATERIALIZED VIEW tid_matview AS SELECT a FROM tid_tab;
+SELECT currtid('tid_matview'::regclass::oid, '(0,1)'::tid); -- fails
+SELECT currtid2('tid_matview'::text, '(0,1)'::tid); -- fails
+INSERT INTO tid_tab VALUES (1);
+REFRESH MATERIALIZED VIEW tid_matview;
+SELECT currtid('tid_matview'::regclass::oid, '(0,1)'::tid); -- ok
+SELECT currtid2('tid_matview'::text, '(0,1)'::tid); -- ok
+DROP MATERIALIZED VIEW tid_matview;
+TRUNCATE tid_tab;
+
+-- Sequence
+CREATE SEQUENCE tid_seq;
+SELECT currtid('tid_seq'::regclass::oid, '(0,1)'::tid); -- ok
+SELECT currtid2('tid_seq'::text, '(0,1)'::tid); -- ok
+DROP SEQUENCE tid_seq;
+
+-- Index, fails with incorrect relation type
+CREATE INDEX tid_ind ON tid_tab(a);
+SELECT currtid('tid_ind'::regclass::oid, '(0,1)'::tid); -- fails
+SELECT currtid2('tid_ind'::text, '(0,1)'::tid); -- fails
+DROP INDEX tid_ind;
+
+-- Partitioned table, no storage
+CREATE TABLE tid_part (a int) PARTITION BY RANGE (a);
+SELECT currtid('tid_part'::regclass::oid, '(0,1)'::tid); -- fails
+SELECT currtid2('tid_part'::text, '(0,1)'::tid); -- fails
+DROP TABLE tid_part;
+
+-- Views
+-- ctid not defined in the view
+CREATE VIEW tid_view_no_ctid AS SELECT a FROM tid_tab;
+SELECT currtid('tid_view_no_ctid'::regclass::oid, '(0,1)'::tid); -- fails
+SELECT currtid2('tid_view_no_ctid'::text, '(0,1)'::tid); -- fails
+DROP VIEW tid_view_no_ctid;
+-- ctid fetched directly from the source table.
+CREATE VIEW tid_view_with_ctid AS SELECT ctid, a FROM tid_tab;
+SELECT currtid('tid_view_with_ctid'::regclass::oid, '(0,1)'::tid); -- fails
+SELECT currtid2('tid_view_with_ctid'::text, '(0,1)'::tid); -- fails
+INSERT INTO tid_tab VALUES (1);
+SELECT currtid('tid_view_with_ctid'::regclass::oid, '(0,1)'::tid); -- ok
+SELECT currtid2('tid_view_with_ctid'::text, '(0,1)'::tid); -- ok
+DROP VIEW tid_view_with_ctid;
+TRUNCATE tid_tab;
+-- ctid attribute with incorrect data type
+CREATE VIEW tid_view_fake_ctid AS SELECT 1 AS ctid, 2 AS a;
+SELECT currtid('tid_view_fake_ctid'::regclass::oid, '(0,1)'::tid); -- fails
+SELECT currtid2('tid_view_fake_ctid'::text, '(0,1)'::tid); -- fails
+DROP VIEW tid_view_fake_ctid;
+
+DROP TABLE tid_tab CASCADE;
signature.asc
Description: PGP signature
