On Mon, May 25, 2020 at 06:29:10PM +0900, Michael Paquier wrote: > Perhaps you are right though, and that we don't need to spend this > much energy into improving the error messages so I am fine to discard > this part. At the end, in order to remove the crashes, you just need > to keep around the two RELKIND_HAS_STORAGE() checks. But I would > rather keep these two to use ereport(ERRCODE_FEATURE_NOT_SUPPORTED) > instead of elog(), and keep the test coverage of the previous patch > (including the tests for the aggregates I noticed were missing). > Would you be fine with that?
And this means the attached. Thoughts are welcome. -- Michael
diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c
index 4ce8375eab..aa55ba7a81 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"
@@ -349,6 +350,12 @@ currtid_for_view(Relation viewrel, ItemPointer tid)
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 +385,13 @@ 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 relation \"%s.%s\"",
+ get_namespace_name(RelationGetNamespace(rel)),
+ RelationGetRelationName(rel))));
+
ItemPointerCopy(tid, result);
snapshot = RegisterSnapshot(GetLatestSnapshot());
@@ -391,6 +405,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 +435,13 @@ 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 relation \"%s.%s\"",
+ 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..e7e0d74780
--- /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 relation "public.tid_part"
+SELECT currtid2('tid_part'::text, '(0,1)'::tid); -- fails
+ERROR: cannot look at latest visible tid for 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: currtid cannot handle views with no CTID
+SELECT currtid2('tid_view_no_ctid'::text, '(0,1)'::tid); -- fails
+ERROR: currtid cannot handle views with no 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: ctid isn't of type TID
+SELECT currtid2('tid_view_fake_ctid'::text, '(0,1)'::tid); -- fails
+ERROR: ctid isn't of type TID
+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
