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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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

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

Neil Conway <neilc(at)samurai(dot)com> 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

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 Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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 Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support