Re: [PATCHES] Moving snapshot code around

2008-03-26 Thread Alvaro Herrera 
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

2008-03-26 Thread Alvaro Herrera 
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

2008-03-26 Thread Tom Lane 
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

2008-03-26 Thread Alvaro Herrera 
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

2008-03-26 Thread Tom Lane 
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

2008-03-26 Thread Alvaro Herrera 
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

2008-03-25 Thread Tom Lane 
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

2008-03-25 Thread Neil Conway 
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

2008-03-20 Thread Simon Riggs 
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

2008-03-18 Thread Alvaro Herrera 
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 Cop