GetTransactionSnapshot() in enum.c
Hi,
ISTM that we shouldn't use GetTransactionSnapshot() in enum.c but
GetLatestSnapshot() in <= 9.3 and NULL/GetCatalogSnapshot() > 9.3.
typecache.c's usage was converted to GetLatestSnapshot() but enum.c's
was not.
I don't seem to have full mental capacity right now, but I think the
current usage could cause problems with a range type index over a enum
column. Index predicates might also be problematic.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
ISTM that we shouldn't use GetTransactionSnapshot() in enum.c but
GetLatestSnapshot() in <= 9.3 and NULL/GetCatalogSnapshot() > 9.3.
typecache.c's usage was converted to GetLatestSnapshot() but enum.c's
was not.
That was intentional, see the comments for commit
9ad45c18b6c8d03ce18a26223eb0d15e900c7a2c.
Possibly we should rethink this in HEAD given that we don't do SnapshotNow
scans anymore, but I'm disinclined to do so in back branches.
BTW, I notice that the MVCC-catalog-scans patch summarily asserts that
RenumberEnumType no longer poses any concurrency hazards. I doubt that's
true: isn't it still possible that pg_enum rows acquired through the
syscaches will have inconsistent enumsortorder values, if they were
read at different times? If you want to examine enumsortorder, you really
need to be comparing rows you know were read with the *same* snapshot.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 19, 2013 at 1:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@2ndquadrant.com> writes:
ISTM that we shouldn't use GetTransactionSnapshot() in enum.c but
GetLatestSnapshot() in <= 9.3 and NULL/GetCatalogSnapshot() > 9.3.typecache.c's usage was converted to GetLatestSnapshot() but enum.c's
was not.That was intentional, see the comments for commit
9ad45c18b6c8d03ce18a26223eb0d15e900c7a2c.Possibly we should rethink this in HEAD given that we don't do SnapshotNow
scans anymore, but I'm disinclined to do so in back branches.BTW, I notice that the MVCC-catalog-scans patch summarily asserts that
RenumberEnumType no longer poses any concurrency hazards. I doubt that's
true: isn't it still possible that pg_enum rows acquired through the
syscaches will have inconsistent enumsortorder values, if they were
read at different times? If you want to examine enumsortorder, you really
need to be comparing rows you know were read with the *same* snapshot.
Good point, I missed that. Here's a proposed comment patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
pg_enum_now.patchapplication/octet-stream; name=pg_enum_now.patchDownload
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index 88711e9..35899b4 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -462,9 +462,22 @@ restart:
* Renumber existing enum elements to have sort positions 1..n.
*
* We avoid doing this unless absolutely necessary; in most installations
- * it will never happen. Before we had MVCC catalog scans, this function
- * posed various concurrency hazards. It no longer does, but it's still
- * extra work, so we don't do it unless it's needed.
+ * it will never happen. The reason is that updating existing pg_enum
+ * entries creates hazards for other backends that are concurrently reading
+ * pg_enum. Although system catalog scans now use MVCC semantics, the
+ * syscache machinery might read different pg_enum entries under different
+ * snapshots, so some other backend might get confused about the proper
+ * ordering if a concurrent renumbering occurs.
+ *
+ * We therefore make the following choices:
+ *
+ * 1. Any code that is interested in the enumsortorder values MUST read
+ * all the relevant pg_enum entries with a single MVCC snapshot, or else
+ * acquire lock on the enum type to prevent concurrent execution of
+ * AddEnumLabel().
+ *
+ * 2. Code that is not examining enumsortorder can use a syscache
+ * (for example, enum_in and enum_out do so).
*/
static void
RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems)
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Aug 19, 2013 at 1:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
BTW, I notice that the MVCC-catalog-scans patch summarily asserts that
RenumberEnumType no longer poses any concurrency hazards. I doubt that's
true: isn't it still possible that pg_enum rows acquired through the
syscaches will have inconsistent enumsortorder values, if they were
read at different times? If you want to examine enumsortorder, you really
need to be comparing rows you know were read with the *same* snapshot.
Good point, I missed that. Here's a proposed comment patch.
Looks sane to me.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 26, 2013 at 4:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Aug 19, 2013 at 1:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
BTW, I notice that the MVCC-catalog-scans patch summarily asserts that
RenumberEnumType no longer poses any concurrency hazards. I doubt that's
true: isn't it still possible that pg_enum rows acquired through the
syscaches will have inconsistent enumsortorder values, if they were
read at different times? If you want to examine enumsortorder, you really
need to be comparing rows you know were read with the *same* snapshot.Good point, I missed that. Here's a proposed comment patch.
Looks sane to me.
Thanks for the review. Committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers