Re: [PATCHES] Moving snapshot code around
Tom Lane wrote:
I think thinking of snapshot.h as an external interface is
wrongheaded. In the proposed refactoring, snapshot.h is concerned with
snapshot *management* (creating, copying, deleting) while tqual.h is
concerned with tuple visibility testing (which requires a snapshot as an
input, but doesn't do any management). They're really entirely
orthogonal concerns.
Agreed, it makes a lot more sense considered in this light. I renamed
snapshot.{c,h} into snapmgmt.{c,h}, hopefully making the intent clearer.
I also separated the definition of the snapshot struct to snapshot.h.
This caused the new snapmgmt.h header be required in more files, but I
don't see this as a problem because it means tqual.h is now less
generally included.
Patch committed that way.
One thing I'm unhappy about is that tqual.h needs to be included in
heapam.h (which is included just about everywhere) just to get the
definition of the HTSU_Result enum, which is a bit useless because it is
only used in three switch statements that contain a default clause
anyway. I propose changing the result type of heap_update, heap_delete
and heap_lock_tuple to a plain int.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
Alvaro Herrera [EMAIL PROTECTED] writes:
Agreed, it makes a lot more sense considered in this light. I renamed
snapshot.{c,h} into snapmgmt.{c,h}, hopefully making the intent clearer.
I'd have gone with snapmgr.h/c for consistency with existing filenames
(bufmgr, lmgr, etc).
One thing I'm unhappy about is that tqual.h needs to be included in
heapam.h (which is included just about everywhere) just to get the
definition of the HTSU_Result enum, which is a bit useless because it is
only used in three switch statements that contain a default clause
anyway. I propose changing the result type of heap_update, heap_delete
and heap_lock_tuple to a plain int.
I don't like that very much. What about just moving the HTSU_Result
enum's declaration somewhere else? Two possibilities are heapam.h
itself, or the new snapshot.h file (which'd then have to be included
by heapam.h, but it seems lightweight enough that that's not too
terrible).
regards, tom lane
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
Tom Lane wrote:
Alvaro Herrera [EMAIL PROTECTED] writes:
Agreed, it makes a lot more sense considered in this light. I renamed
snapshot.{c,h} into snapmgmt.{c,h}, hopefully making the intent clearer.
I'd have gone with snapmgr.h/c for consistency with existing filenames
(bufmgr, lmgr, etc).
Doh! Sorry. We're at the best time for changing the name, since the
file has no history. Shall I?
One thing I'm unhappy about is that tqual.h needs to be included in
heapam.h (which is included just about everywhere) just to get the
definition of the HTSU_Result enum, which is a bit useless because it is
only used in three switch statements that contain a default clause
anyway. I propose changing the result type of heap_update, heap_delete
and heap_lock_tuple to a plain int.
I don't like that very much. What about just moving the HTSU_Result
enum's declaration somewhere else? Two possibilities are heapam.h
itself, or the new snapshot.h file (which'd then have to be included
by heapam.h, but it seems lightweight enough that that's not too
terrible).
Well, heapam.h includes a lot of other headers, so it doesn't look a
good candidate to me. I think snapshot.h is a reasonably good
candidate.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: I'd have gone with snapmgr.h/c for consistency with existing filenames (bufmgr, lmgr, etc). Doh! Sorry. We're at the best time for changing the name, since the file has no history. Shall I? +1 I don't like that very much. What about just moving the HTSU_Result enum's declaration somewhere else? Two possibilities are heapam.h itself, or the new snapshot.h file (which'd then have to be included by heapam.h, but it seems lightweight enough that that's not too terrible). Well, heapam.h includes a lot of other headers, so it doesn't look a good candidate to me. I think snapshot.h is a reasonably good candidate. Works for me. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: I'd have gone with snapmgr.h/c for consistency with existing filenames (bufmgr, lmgr, etc). Doh! Sorry. We're at the best time for changing the name, since the file has no history. Shall I? +1 Done. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: I don't like that very much. What about just moving the HTSU_Result enum's declaration somewhere else? Two possibilities are heapam.h itself, or the new snapshot.h file (which'd then have to be included by heapam.h, but it seems lightweight enough that that's not too terrible). Well, heapam.h includes a lot of other headers, so it doesn't look a good candidate to me. I think snapshot.h is a reasonably good candidate. Works for me. This part done too. Thanks for the input. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
On Tue, 2008-03-18 at 16:19 -0300, Alvaro Herrera wrote: The other approach, of course, is to just keep all the code in tqual.c and not create a separate module at all. Opinions? I prefer to keep them separate, but I'm not wedded to it if there's any strong reason not to do it. Also, the line is currently blurred because some users of snapshots mess with the internals directly (setting snapshot-curcid), but we could clean that up and make it so that those become external interface users too. Sounds like a good idea to me -- +1 on keeping the code in two separate files, and moving snapshot users toward using the external interface. Given that there's no functional change here, I don't see anything to stop this patch being applied sooner rather than later... -Neil -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
Neil Conway [EMAIL PROTECTED] writes: On Tue, 2008-03-18 at 16:19 -0300, Alvaro Herrera wrote: The other approach, of course, is to just keep all the code in tqual.c and not create a separate module at all. Opinions? I prefer to keep them separate, but I'm not wedded to it if there's any strong reason not to do it. Also, the line is currently blurred because some users of snapshots mess with the internals directly (setting snapshot-curcid), but we could clean that up and make it so that those become external interface users too. Sounds like a good idea to me -- +1 on keeping the code in two separate files, and moving snapshot users toward using the external interface. I think thinking of snapshot.h as an external interface is wrongheaded. In the proposed refactoring, snapshot.h is concerned with snapshot *management* (creating, copying, deleting) while tqual.h is concerned with tuple visibility testing (which requires a snapshot as an input, but doesn't do any management). They're really entirely orthogonal concerns. Some callers will need one, some will need the other, a few might need both. But you could very much argue that tqual.c depends on snapshot.c not the other way around, which makes tqual the external party if you ask me. With the exception of the outputs in SnapshotDirty testing, tqual.h's operations would be read-only as far as snapshots are concerned. Not sure if the read-only vs read-write distinction is helpful here, but that's pretty much how it seems to be breaking down. I don't have any particular objection to the factoring as proposed, only to the way it's described. I am wondering a bit if the Snapshot struct should be defined in a third file that's included by both of these, rather than making either .h file depend on the other. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Moving snapshot code around
On Tue, 2008-03-18 at 16:19 -0300, Alvaro Herrera wrote: I'm playing with the snapshot code to create a new module to stash used snapshots and refcount them. Sounds good. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] Moving snapshot code around
Hi, I'm playing with the snapshot code to create a new module to stash used snapshots and refcount them. It occured to me that a first easy step is to separate the relevant code from tqual.c into a new file, snapshot.c, and split tqual.h in two creating snapshot.h. Basically the internals of snapshots are now in tqual.c/h, and the external interface is snapshot.c/h. The nice thing about it is that most users of snapshots only need the external interface, so most details can remain behind tqual.h which is now a seldom-included header. (The bad news is that the widely used heapam.h still has to include it, because it needs the HTSU_Result enum, so tqual.h is still indirectly included in a lot of places. I think I can easily move the enum definition to snapshot.h but it seems weird.) So here's a patch to do this. It just moves code around -- there's no extra functionality here. The other approach, of course, is to just keep all the code in tqual.c and not create a separate module at all. Opinions? I prefer to keep them separate, but I'm not wedded to it if there's any strong reason not to do it. Also, the line is currently blurred because some users of snapshots mess with the internals directly (setting snapshot-curcid), but we could clean that up and make it so that those become external interface users too. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. *** 00head/src/backend/access/transam/subtrans.c 2008-01-09 13:04:32.0 -0300 --- 01snapshot/src/backend/access/transam/subtrans.c 2008-03-18 12:13:18.0 -0300 *** *** 31,37 #include access/slru.h #include access/subtrans.h #include access/transam.h ! #include utils/tqual.h /* --- 31,37 #include access/slru.h #include access/subtrans.h #include access/transam.h ! #include utils/snapshot.h /* *** 00head/src/backend/access/transam/transam.c 2008-03-14 09:54:13.0 -0300 --- 01snapshot/src/backend/access/transam/transam.c 2008-03-18 12:04:01.0 -0300 *** *** 22,28 #include access/clog.h #include access/subtrans.h #include access/transam.h ! #include utils/tqual.h /* --- 22,28 #include access/clog.h #include access/subtrans.h #include access/transam.h ! #include utils/snapshot.h /* *** 00head/src/backend/catalog/catalog.c 2008-02-26 16:42:19.0 -0300 --- 01snapshot/src/backend/catalog/catalog.c 2008-03-18 15:06:02.0 -0300 *** *** 38,43 --- 38,44 #include storage/fd.h #include utils/fmgroids.h #include utils/relcache.h + #include utils/tqual.h #define OIDCHARS 10 /* max chars printed by %u */ *** 00head/src/backend/commands/explain.c 2008-01-09 13:04:32.0 -0300 --- 01snapshot/src/backend/commands/explain.c 2008-03-18 14:59:28.0 -0300 *** *** 31,36 --- 31,37 #include utils/guc.h #include utils/lsyscache.h #include utils/tuplesort.h + #include utils/tqual.h /* Hook for plugins to get control in ExplainOneQuery() */ *** 00head/src/backend/commands/variable.c 2008-01-09 13:04:33.0 -0300 --- 01snapshot/src/backend/commands/variable.c 2008-03-18 12:05:09.0 -0300 *** *** 25,31 #include utils/acl.h #include utils/builtins.h #include utils/syscache.h ! #include utils/tqual.h #include mb/pg_wchar.h /* --- 25,31 #include utils/acl.h #include utils/builtins.h #include utils/syscache.h ! #include utils/snapshot.h #include mb/pg_wchar.h /* *** 00head/src/backend/utils/adt/txid.c 2008-01-09 13:04:40.0 -0300 --- 01snapshot/src/backend/utils/adt/txid.c 2008-03-18 14:58:34.0 -0300 *** *** 26,31 --- 26,32 #include funcapi.h #include libpq/pqformat.h #include utils/builtins.h + #include utils/tqual.h #ifndef INT64_IS_BUSTED *** 00head/src/backend/utils/time/Makefile 2008-02-20 00:53:22.0 -0300 --- 01snapshot/src/backend/utils/time/Makefile 2008-03-18 11:36:21.0 -0300 *** *** 12,17 top_builddir = ../../../.. include $(top_builddir)/src/Makefile.global ! OBJS = combocid.o tqual.o include $(top_srcdir)/src/backend/common.mk --- 12,17 top_builddir = ../../../.. include $(top_builddir)/src/Makefile.global ! OBJS = combocid.o tqual.o snapshot.o include $(top_srcdir)/src/backend/common.mk *** 00head/src/backend/utils/time/snapshot.c 1969-12-31 21:00:00.0 -0300 --- 01snapshot/src/backend/utils/time/snapshot.c 2008-03-18 14:38:24.0 -0300 *** *** 0 --- 1,176 + /*- + * snapshot.c + * PostgreSQL snapshot management code. + * + * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + *
