Обсуждение: why does enum_endpoint call GetTransactionSnapshot()?
The comments say say that we must use "an" MVCC snapshot here, but
they don't explain why it should be one retrieved via
GetTransactionSnapshot() rather than GetActiveSnapshot() or
GetLatestSnapshot() or GetCatalogSnapshot(). The comments also refer
the reader to catalog/pg_enum.c for more details, but the only
information I can find in pg_enum.c just says that if reading multiple
values for the same enum, the same snapshot should be used for all of
them. That's not an issue here, since we're only reading one value.
The behavior can't be chalked up to a desire for MVCC compliance,
because it updates the transaction snapshot mid-query:
rhaas=# create type color as enum ('red');
CREATE TYPE
rhaas=# select distinct enum_last(NULL::color) from
generate_series(1,1000000) g;enum_last
-----------red
(1 row)
rhaas=# select distinct enum_last(NULL::color) from
generate_series(1,1000000) g;
-- while this is running, do "alter type color add value 'green';" in
another window
enum_last
-----------redgreen
(2 rows)
I think this is probably a holdover from before the death of
SnapshotNow, and that we should just pass NULL to
systable_beginscan_ordered() here, the same as we do for other catalog
accesses. Barring objections, I'll go make that change.
If there's some remaining reason for doing it this way, we should add
comments explaining what it is.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I think this is probably a holdover from before the death of
> SnapshotNow, and that we should just pass NULL to
> systable_beginscan_ordered() here, the same as we do for other catalog
> accesses. Barring objections, I'll go make that change.
Seems reasonable to me, but why are you only looking at that one and
not the identical code in enum_range_internal()?
regards, tom lane
On Sat, Feb 14, 2015 at 5:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I think this is probably a holdover from before the death of >> SnapshotNow, and that we should just pass NULL to >> systable_beginscan_ordered() here, the same as we do for other catalog >> accesses. Barring objections, I'll go make that change. > > Seems reasonable to me, but why are you only looking at that one and > not the identical code in enum_range_internal()? I noticed that one after hitting send. I think we should change them both. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Feb 14, 2015 at 05:29:43PM -0500, Robert Haas wrote: > On Sat, Feb 14, 2015 at 5:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> I think this is probably a holdover from before the death of > >> SnapshotNow, and that we should just pass NULL to > >> systable_beginscan_ordered() here, the same as we do for other catalog > >> accesses. Barring objections, I'll go make that change. > > > > Seems reasonable to me, but why are you only looking at that one and > > not the identical code in enum_range_internal()? > > I noticed that one after hitting send. I think we should change them both. Any news on this? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Tue, Apr 28, 2015 at 9:28 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Sat, Feb 14, 2015 at 05:29:43PM -0500, Robert Haas wrote: >> On Sat, Feb 14, 2015 at 5:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> > Robert Haas <robertmhaas@gmail.com> writes: >> >> I think this is probably a holdover from before the death of >> >> SnapshotNow, and that we should just pass NULL to >> >> systable_beginscan_ordered() here, the same as we do for other catalog >> >> accesses. Barring objections, I'll go make that change. >> > >> > Seems reasonable to me, but why are you only looking at that one and >> > not the identical code in enum_range_internal()? >> >> I noticed that one after hitting send. I think we should change them both. > > Any news on this? Done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company