Re: Snapshot synchronization, again...

Here is a new version of the patch incorporating most of Noah's
suggestions. The patch now also adds documentation. Since I couldn't
really find a suitable section to document the two new functions, I
added a new one for now. Any better ideas where it should go?

On Thu, Jan 20, 2011 at 1:37 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> Just to clarify, I was envisioning something like:
>
> typedef struct { bool valid; char value[16]; } snapshotChksum;
>
> I don't object to the way you've done it, but someone else might not like the
> extra marshalling between text and binary.  Your call.

I didn't like it in the beginning but later on I adopted your
proposal. I have also changed the locking to be more natural. Even
though we don't really need it, I am now grabbing shared ProcArrayLock
for any reads of shared memory and exclusive lock for writes. Of
course no additional lock is taken if the feature is not used.

> You're right.  Then consider "VALUES (pg_import_snapshot('...'), (SELECT
> count(*) from t))" at READ COMMITTED.  It works roughly as I'd guess; the
> subquery uses the imported snapshot.  If I flip the two expressions and do
> "VALUES ((SELECT count(*) from t), pg_import_snapshot('...'))", the subquery
> uses the normal snapshot.  That makes sense, but I can't really see a use case
> for it.  A warning, like your code has today, seems appropriate.

Yeah, that would do what you wanted to illustrate but it truely cannot
be considered the standard use case :-)

>> > Is it valid to scribble directly on snapshots like this?
>> I figured that previously executed code still has references pointing
>> to the snapshots and so we cannot just push a new snapshot on top but
>> really need to change the memory where they are pointing to.
> Okay.  Had no special reason to believe otherwise, just didn't know.

This is one part where I'd like someone more experienced than me look into it.

> Thanks; that was handy.  One thing I noticed is that the second "SELECT * FROM
> kidseen" yields zero rows instead of yielding "ERROR:  relation "kidseen" does
> not exist".  I changed things around per the attached "test-drop.pl", such that
> the table is already gone in the ordinary snapshot, but still visible to the
> imported snapshot.  Note how the pg_class row is visible, but an actual attempt
> to query the table fails.  Must some kind of syscache invalidation follow the
> snapshot alteration to make this behave normally?

As discussed with Noah off-list this is just not an MVCC safe
operation. You could hit on this in a regular SERIALIZABLE transaction
as well: Somebody else can delete a table and you won't be able to
access it anymore. This is also the reason why pg_dump in the
beginning gets a shared lock on every table that it will dump later
on.

Thanks for the review Noah,

Joachim