SEO

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


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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 ?

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

"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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

On 9/4/07, Florian G. Pflug <fgp@phlo.org> wrote:
>
>
> I don't think so. The interactions should be pretty minimal.



Thats good news!


Thanks,
Pavan


--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

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


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

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

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate