Thread: Lazy xid assignment V4

Hi

Here is an updated patch, following the discussion.
The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch
(I seems I still can't get attachments through to this list)

Most changes are just small fixes and tweaks. Those are
.) Introduced %v for log_line_prefix
.) I missed a few places when I renamed ResourceOwnerId to
    VirtualTransactionId. (In lock.c/.h and lmgr.c/.h +
    a few comments). Should be fixed now.
.) Fixed some typos and outdated-ness in comments.

Two changes are a bit larger

1) 2PC was broken in V3. I added code that skips LOCKTYPE_VIRTUALTRANSACTION
locks when writing the locks to the 2PC state file, but I didn't
add the same exception to the code that reassigns the locks to
a dummy PGROC afterwards. So the locks weren't released at PREPARE
time. Fixed now.

2) The more I thought about the sessionId, the less I liked it.
It adds a new per-backend identifier, beside pid and BackendId,
only to than *still* have wraparound issues. That seems a bit
stupid.

I therefore got rid of the sessionId completely, and replaced it
with MyBackendId. To guarantee that VirtualTransactionIds are
not reused too quickly, I added an array nextLocalTransactionIds
to shared memory, with one entry per backendId. That seems much cleaner -
it doesn't add yet another per-backend identifier, doesn't need
yet another variable initialized at backend start, and solved the
wraparound problem too.

It *does* cost a bit of shared memory, but not much. For
max_backends=2048 the array is as large as *one* shared buffer.

It doesn't need any locking, either, because each backend
only ever touches it's own entry in the array.

Because the backendId part of VirtualTransactionIds is now
always a rather small number (<= MaxBackends), and might
be interesting to know, I changed the string representation to
show the backendId in decimal notation.

greetings, Florian Pflug

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> On 9/4/07, Florian G. Pflug <fgp@phlo.org> wrote:
>> Here is an updated patch, following the discussion.
>> The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch
>> (I seems I still can't get attachments through to this list)
>>
> I haven't been able to follow the discussions here, but do I need to worry
> about its interaction with HOT ?

There will likely be some minor patch-merging problems, but AFAICT there
are no semantic conflicts to worry about.

            regards, tom lane

Pavan Deolasee wrote:
> On 9/4/07, Florian G. Pflug <fgp@phlo.org> wrote:
>> Hi
>>
>> Here is an updated patch, following the discussion.
>> The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch
>> (I seems I still can't get attachments through to this list)
>>
>>
> I haven't been able to follow the discussions here, but do I need to worry
> about its interaction with HOT ?

I don't think so. The interactions should be pretty minimal.

Any transaction that actually modifies a tuple will be assigned an xid
(That happens automatically if you call Get{Current|Top}TransactionId() ).
So those work just as they used to, only that the transaction id
is assigned a bit later. But GetSnapshotData() and friends deal correctly
with that.

It's only purely read-only transactions that behave a bit different.
AFAIK, with HOT, those still might try to prune and defragment a page - but they
won't need an xid for that I guess. Even if they did - calling
Get{Current|Top}TransctionId will assign one, so things should just work.

If you scan the procarray, and then wait for some of the transactions you found,
you might need to wait on the virtual transaction id instead of the regular
transaction id (because that might be InvalidTransactionId) - just as
CREATE INDEX CONCURRENTLY does. But the infrastructure for that is already in
place, so thats a rather trivial modification.

If HOT does change the second waiting phase of CREATE INDEX CONCURRENTLY to
use the new xidcreate (or however that is called) field, than that probably
replaces my changes to that code. That's fine - I knew that HOT might make
my changes unnecessary when I coded them, but I didn't want my patch to have a
dependency on HOT - that seemed to complicate things unnecessarily. And I
wasn't sure if HOT changes only normal CREATE INDEX, or both CREATE INDEX
and CREATE INDEX CONCURRENTLY.

greetings, Florian Pflug

fgp@phlo.org ("Florian G. Pflug") writes:
> Pavan Deolasee wrote:
>> On 9/4/07, Florian G. Pflug <fgp@phlo.org> wrote:
>>> Hi
>>>
>>> Here is an updated patch, following the discussion.
>>> The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch
>>> (I seems I still can't get attachments through to this list)
>>>
>>>
>> I haven't been able to follow the discussions here, but do I need to worry
>> about its interaction with HOT ?
>
> I don't think so. The interactions should be pretty minimal.

Similarly, does it seem likely that Slony-I users would need to worry
about this?

From what I have been seeing, I don't think so, because the
transactions that cause the XIDs to get generated that are of interest
will all be write transactions, and hence exempt from this:

- User transactions that are writing to tables will be exempt :-).

- On an origin node, slon connections that write out SYNC events every
  so often are, obviously, write transactions that will again be exempt.

- Transactions that read data from a provider could be affected, as
  they are indeed read-only, but their XIDs aren't of interest.

If I speculate right, then there's not much here to worry about.  But
best to check...  And we'll certainly be testing as this gets
applied...
--
let name="cbbrowne" and tld="acm.org" in name ^ "@" ^ tld;;
http://linuxdatabases.info/info/finances.html
Signs of a Klingon Programmer - 3. "This  machine is GAGH! I need dual
Pentium processors if I am to do battle with this code!"

Chris Browne wrote:
> Similarly, does it seem likely that Slony-I users would need to worry
> about this?
No.. it should have zero negative effects for Slony-I. In fact, it will
be an advantage in some cases I think. I remember something about
troubles with Slony-I if the in-use xids on a intermediate subscribe
(one that also acts as a origin) drift too bar away from those on the
master. If that still is an issue, than lazy xid assignment might help
a bit - it might reduce xid consumption on that intermediate subscriber.

In general, from a user's point of view, you only see a different if
you look at pg_locks - you will now see NULLs in the transaction
column, and might need to look at virtualtransaction for some use-cases.

[ thinking ] It's been quite a time since I last worked with slony - but
isn't there some code that tried to prevent blocking other queries by
looking at pg_locks? Or was that before you could conditionally acquire
a lock using SQL? Or am I totally mistaken? Anyway, if you *do* scan
pg_locks, than you might want to check those parts of the code.

greetings, Florian Pflug

"Florian G. Pflug" <fgp@phlo.org> writes:
> Here is an updated patch, following the discussion.
> The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch

I've been working through this, and found a couple items that seem like
judgment calls:

* Is there a good reason for formatting VXIDs as %d/%X rather than
%d/%u?  There are no other columns in pg_locks that we use hex for,
so this seems a bit jarring.

* What is the rationale for keeping the transaction column in pg_locks?
The useful uniqueness/join column will be virtualtransaction.  I don't
see a very strong backwards-compatibility argument for keeping it,
because any code that depends on it being there probably thinks it's a
unique key, which it cannot be anymore.

One could actually make an argument to rename the virtualtransaction
column as transaction.  This will avoid breaking client code that thinks
that the transaction column is a unique key and isn't too wedded to the
assumption that the contents look like an integer.  If it is so wedded,
it's most likely busted anyway by the possibility that the column is
null...

Comments?

            regards, tom lane

Florian G. Pflug wrote:
> 1) 2PC was broken in V3. I added code that skips
> LOCKTYPE_VIRTUALTRANSACTION
> locks when writing the locks to the 2PC state file, but I didn't
> add the same exception to the code that reassigns the locks to
> a dummy PGROC afterwards. So the locks weren't released at PREPARE
> time. Fixed now.

Let me check if I got this right:

We only use the lock on virtual transaction id in CREATE INDEX
CONCURRENTLY, to wait until everyone that might insert to the table sees
the new index. Releasing the virtual transaction id right away at
PREPARE TRANSACTION, instead of reassigning it to the dummy PGPROC, is
ok because the transaction can't insert anything to the table after
PREPARE TRANSACTION.

Sounds valid to me, but better add some comments to note that the lock
is released early, in case it's going to be used for some other purpose
in the future.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Tom Lane wrote:
> "Florian G. Pflug" <fgp@phlo.org> writes:
>> Here is an updated patch, following the discussion.
>> The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch
>
> I've been working through this, and found a couple items that seem like
> judgment calls:
>
> * Is there a good reason for formatting VXIDs as %d/%X rather than
> %d/%u?  There are no other columns in pg_locks that we use hex for,
> so this seems a bit jarring.

The reason was the desire not to bloat the string length unnecessarily.
Since the first part is now %d anyways, it only really saves 2 bytes, though...

> * What is the rationale for keeping the transaction column in pg_locks?
> The useful uniqueness/join column will be virtualtransaction.  I don't
> see a very strong backwards-compatibility argument for keeping it,
> because any code that depends on it being there probably thinks it's a
> unique key, which it cannot be anymore.
>
> One could actually make an argument to rename the virtualtransaction
> column as transaction.  This will avoid breaking client code that thinks
> that the transaction column is a unique key and isn't too wedded to the
> assumption that the contents look like an integer.  If it is so wedded,
> it's most likely busted anyway by the possibility that the column is
> null...

Wouldn't code that assumes that transaction is "not null" be broken already,
because of session locks?

I left it there because they only way to get it back if we remove it is to
join pg_locks on itself. Thats quite a lot of work - both in terms of typing
and CPU cycles to just get that one column.

I felt that if we remove the holder's xid from pg_locks, we ought to add it
pg_stat_activity, probably along with the virtual transaction id. I actually
wanted to do this, but then didn't because currently pg_stat_activity is rather
tightly bound to the stats collector. Adding random other values seemes like a
bit of a hack...

However, none of these are very strong reasons - certainly weaker than doing
what ensures to cause the least confusion. I'm therefore starting to think that
we should remove transaction, and keep the name virtualtransaction for the
VXID. That will ensure that clients who *do* rely on pg_locks and the
"transaction" column (which will be few, I guess) at least fail early and
visibly, instead of producing bogus results...

If we go ahead, and rename virtualtransaction to transaction, I think we should
at least put some non-numeric character in front of the virtualtransaction.
Most language's string-to-integer functions will happily convert
"<number><string>" to the integer <number>. So if they indeed treat
virtualtransaction as something int-like, they'd silently use only the backendId
instead of the full VXID.

greetings, Florian Pflug

Heikki Linnakangas wrote:
> Florian G. Pflug wrote:
>> 1) 2PC was broken in V3. I added code that skips
>> LOCKTYPE_VIRTUALTRANSACTION
>> locks when writing the locks to the 2PC state file, but I didn't
>> add the same exception to the code that reassigns the locks to
>> a dummy PGROC afterwards. So the locks weren't released at PREPARE
>> time. Fixed now.
>
> Let me check if I got this right:
>
> We only use the lock on virtual transaction id in CREATE INDEX
> CONCURRENTLY, to wait until everyone that might insert to the table sees
> the new index. Releasing the virtual transaction id right away at
> PREPARE TRANSACTION, instead of reassigning it to the dummy PGPROC, is
> ok because the transaction can't insert anything to the table after
> PREPARE TRANSACTION.

Yes. Or to put it another way: We use the transaction id to wait for
transactions that did on-disk changes, and the virtual transaction id
to wait for transactions only found in shared memory. Since xacts mostly
vanishe from shmem at PREPARE time, we drop the lock on the VXID,
but keep the one on XID.

> Sounds valid to me, but better add some comments to note that the lock
> is released early, in case it's going to be used for some other purpose
> in the future.
Yeah, more comments are always a Good Thing I guess.

greetings, Florian Pflug

"Florian G. Pflug" <fgp@phlo.org> writes:
> However, none of these are very strong reasons - certainly weaker than
> doing what ensures to cause the least confusion. I'm therefore
> starting to think that we should remove transaction, and keep the name
> virtualtransaction for the VXID. That will ensure that clients who
> *do* rely on pg_locks and the "transaction" column (which will be few,
> I guess) at least fail early and visibly, instead of producing bogus
> results...

Barring other objections, I'll do it that way.

            regards, tom lane

"Florian G. Pflug" <fgp@phlo.org> writes:
> Here is an updated patch, following the discussion.
> The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch
> (I seems I still can't get attachments through to this list)

Applied with revisions --- mostly cosmetic, but there were a couple of
things that seemed really broken.  In particular, I didn't trust at all
your use of struct assignment to copy VXIDs into and out of PGPROC.
I believe that the C compiler is entitled to implement struct assignment
by bytewise memcpy, for instance, and so it wouldn't be atomic.  The
LocalTransactionId can be fetched or stored atomically, but you have to
write it as an integer assignment to be sure that that's what happens.
I also fixed sequence.c to not force XID assignment --- it can perfectly
well use the LocalTransactionId for what it's doing.

Also, I didn't add the proposed regression test, as it seems much too
fragile --- concurrent autovacuum activity would make it fail, for
instance.

There are a couple of loose ends, which I'll post about separately
on -hackers.

            regards, tom lane

Tom Lane wrote:
> "Florian G. Pflug" <fgp@phlo.org> writes:
>> Here is an updated patch, following the discussion.
>> The patch can be found at: http://soc.phlo.org/lazyxidassign.v4.patch
>> (I seems I still can't get attachments through to this list)
>
> Applied with revisions --- mostly cosmetic, but there were a couple of
> things that seemed really broken.  In particular, I didn't trust at all
> your use of struct assignment to copy VXIDs into and out of PGPROC.
> I believe that the C compiler is entitled to implement struct assignment
> by bytewise memcpy, for instance, and so it wouldn't be atomic.  The
> LocalTransactionId can be fetched or stored atomically, but you have to
> write it as an integer assignment to be sure that that's what happens.
Ah, OK - I wasn't aware of that.

> I also fixed sequence.c to not force XID assignment --- it can perfectly
> well use the LocalTransactionId for what it's doing.
Full ack. I must have missed that user for GetTopTransactionId().

Anyway, thanks for fixing these things.

> Also, I didn't add the proposed regression test, as it seems much too
> fragile --- concurrent autovacuum activity would make it fail, for
> instance.
I was aware that they are fragile, but not that they are *that* fragile ;-)
Anyway they have fullfilled their duty during development, so it's only
fair to let them go now...

Thanks to all of you who helped with making this happen!

greetings, Florian Pflug

fgp@phlo.org ("Florian G. Pflug") writes:
> Chris Browne wrote:
>> Similarly, does it seem likely that Slony-I users would need to worry
>> about this?
> No.. it should have zero negative effects for Slony-I. In fact, it will
> be an advantage in some cases I think. I remember something about
> troubles with Slony-I if the in-use xids on a intermediate subscribe
> (one that also acts as a origin) drift too bar away from those on the
> master. If that still is an issue, than lazy xid assignment might help
> a bit - it might reduce xid consumption on that intermediate subscriber.

The problem isn't usually the growth of XID numbers, but more the
general notion that the open transactions tend to prevent successful
vacuums from taking place on tables like pg_listener.  The pointed
issues with pg_listener has, we think, been mostly resolved, as
Slony-I has been getting less aggressive about generating dead tuples
there.

During initial subscription time, there is a pretty big issue, for
very large replicas as there is a single big, long-running transaction
running on the origin (the transaction pulling initial table data);
that is one "big" XID, and it has history of adversely affecting
application of replication data during the catch-up period.  If
flurries of read-only transactions are no longer generating XIDs that
are being included in snapshot information, that *may* be something of
a help; for our version 2, there is already a change that excludes
XIDs for rolled-back transactions, which gets it mostly around the
issues with the Big Initial-Subscription-Related Transaction.

> In general, from a user's point of view, you only see a different if
> you look at pg_locks - you will now see NULLs in the transaction
> column, and might need to look at virtualtransaction for some use-cases.
>
> [ thinking ] It's been quite a time since I last worked with slony - but
> isn't there some code that tried to prevent blocking other queries by
> looking at pg_locks? Or was that before you could conditionally acquire
> a lock using SQL? Or am I totally mistaken? Anyway, if you *do* scan
> pg_locks, than you might want to check those parts of the code.

There are no references to pg_locks, unless there's some other view
that references it, so that's good news :-).
--
"cbbrowne","@","linuxdatabases.info"
http://www3.sympatico.ca/cbbrowne/rdbms.html
"Windows 98  Roast Specialty Blend  coffee beans - just  like ordinary
gourmet coffee except that processing is rushed to leave in the insect
larvae.  Also sold under the ``Chock Full o' Bugs'' brand name..."

On Wednesday 05 September 2007 12:56, Tom Lane wrote:
> "Florian G. Pflug" <fgp@phlo.org> writes:
> > However, none of these are very strong reasons - certainly weaker than
> > doing what ensures to cause the least confusion. I'm therefore
> > starting to think that we should remove transaction, and keep the name
> > virtualtransaction for the VXID. That will ensure that clients who
> > *do* rely on pg_locks and the "transaction" column (which will be few,
> > I guess) at least fail early and visibly, instead of producing bogus
> > results...
>

Reading the docs, it says "Every transaction holds an exclusive lock on its
virtual transaction ID for its entire duration. If a permanent ID is assigned
to the transaction (which normally happens only if the transaction changes
the state of the database), it also holds an exclusive lock on its permanent
transaction ID until it ends."

ISTM that by removing the transaction column, there is no way to see the XID
for relations thats have been updated (which by definition will have locks on
them).  Am I mis-reading the docs, or have we lost that functionality?

--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

Robert Treat <xzilla@users.sourceforge.net> writes:
> ISTM that by removing the transaction column, there is no way to see the XID
> for relations thats have been updated (which by definition will have locks on
> them).  Am I mis-reading the docs, or have we lost that functionality?

Huh?  What do you mean by "XID for relations that have been updated"?
Relations don't have XIDs.

            regards, tom lane

Robert Treat wrote:
> On Wednesday 05 September 2007 12:56, Tom Lane wrote:
>> "Florian G. Pflug" <fgp@phlo.org> writes:
>>> However, none of these are very strong reasons - certainly weaker than
>>> doing what ensures to cause the least confusion. I'm therefore
>>> starting to think that we should remove transaction, and keep the name
>>> virtualtransaction for the VXID. That will ensure that clients who
>>> *do* rely on pg_locks and the "transaction" column (which will be few,
>>> I guess) at least fail early and visibly, instead of producing bogus
>>> results...
>
> Reading the docs, it says "Every transaction holds an exclusive lock on its
> virtual transaction ID for its entire duration. If a permanent ID is assigned
> to the transaction (which normally happens only if the transaction changes
> the state of the database), it also holds an exclusive lock on its permanent
> transaction ID until it ends."
>
> ISTM that by removing the transaction column, there is no way to see the XID
> for relations thats have been updated (which by definition will have locks on
> them).  Am I mis-reading the docs, or have we lost that functionality?

I'm sure sure if that is what you mean - but there were two columns carrying
transaction ids in pg_locks - the first was called transaction*id*, and held
a transaction that either *is* locked or *is* being waited for. The second
was called just transaction, and held the xid of the transaction *holding*
the lock or waiting *for* the lock. Of course, for exclusive locks on xids,
the first and the second xid where always the same, because nobody apart
from the transaction itself ever requests an exclusive lock on it's xid.

Now, the second column is replaced by virtualtransaction, holding the vxid
of the transaction holding the lock. To get the real xid for the *holding*
transaction, you'd have to join pg_locks to self, using the vxid as a join key.

So, in essence, you get the old pg_locks format back by doing
select l1.*, l2.transactionid as "transaction" from pg_locks l1, pg_locks l2
   where l1.vxid = l2.vxid and l2.locktype = 'transaction'
   and l2.mode='exclusive' and l2.granted=true.

Hm.. Maybe we should put that into the docs or into the release notes?

greetings, Florian Pflug

Florian G. Pflug wrote:
>
> So, in essence, you get the old pg_locks format back by doing
> select l1.*, l2.transactionid as "transaction" from pg_locks l1,
> pg_locks l2
>   where l1.vxid = l2.vxid and l2.locktype = 'transaction'
>   and l2.mode='exclusive' and l2.granted=true.
>
> Hm.. Maybe we should put that into the docs or into the release notes?
>
>

or make it a system view?

cheers

andrew

Andrew Dunstan wrote:
> Florian G. Pflug wrote:
>>
>> So, in essence, you get the old pg_locks format back by doing
>> select l1.*, l2.transactionid as "transaction" from pg_locks l1,
>> pg_locks l2
>>   where l1.vxid = l2.vxid and l2.locktype = 'transaction'
>>   and l2.mode='exclusive' and l2.granted=true.
>>
>> Hm.. Maybe we should put that into the docs or into the release notes?
>>
> or make it a system view?
In that case we can just add "transaction" back to pg_locks, and save
the overhead of snapshotting the locking datastructures twice...

The reason against it was that we changed the semantics of the column -
if we just keep it there, people who use it might silently get wrong
results.

I think what we have now is fine for 8.3. Should we remove the
lock on the xid completely in 8.4, we'll have to revisit pg_locks
anway, since than the current views won't show a transactions xid at all.
But let's do that when 8.4 opens, and not now.

greetings, Florian Pflug

Andrew Dunstan <andrew@dunslane.net> writes:
> Florian G. Pflug wrote:
>> So, in essence, you get the old pg_locks format back by doing
>> select l1.*, l2.transactionid as "transaction" from pg_locks l1,
>> pg_locks l2
>> where l1.vxid = l2.vxid and l2.locktype = 'transaction'
>> and l2.mode='exclusive' and l2.granted=true.

You'd want some sort of left join, no doubt, else you're not going to
see transactions that have not got an XID.

> or make it a system view?

That would be a bit silly.  If there's actually still a use-case for the
XID column then we should just put it back.  I don't actually see a
reasonable use-case for it though.  As Florian points out, you can get
it if you really need it --- but that view is already annoyingly wide,
and I'm not eager to burden it with columns that are usually useless.

Also, I still agree with Florian's earlier argument that we should
deliberately break any code that's depending on the transaction column.
Any such code is unlikely to be prepared for the column containing nulls.

            regards, tom lane