On Thu, Dec 4, 2014 at 9:26 PM, Heikki Linnakangas <hlinnakan...@vmware.com> wrote: > On 12/04/2014 01:47 PM, Petr Jelinek wrote: >> >> On 04/12/14 12:26, Heikki Linnakangas wrote: >>> >>> On 12/04/2014 01:16 PM, Petr Jelinek wrote: >>>> >>>> On 04/12/14 10:42, Heikki Linnakangas wrote: >>>>> >>>>> On 12/03/2014 04:54 PM, Alvaro Herrera wrote: >>>>>> >>>>>> ir commit timestamp directly as they commit, >>>>>> or an external transaction c >>>>> >>>>> >>>>> Sorry, I'm late to the party, but here's some random comments on this >>>>> after a quick review: >>>>> >>>>> * The whole concept of a node ID seems to be undocumented, and unused. >>>>> No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type >>>>> is >>>>> dead code too, when a non-default node_id is not set. Please remove, or >>>>> explain how all that's supposed to work. >>>> >>>> >>>> That's API meant to be used by extensions, maybe it would be preferable >>>> if there was SQL API exposed also? >>>> >>>> (It might also make more sense once replication identifiers are >>>> submitted.) >>> >>> >>> Maybe. Hard to tell without any documentation or comments on how it's >>> supposed to work. In unacceptable in its current state. >>> >>> I would prefer to remove it now, and add it back later together with the >>> code that actually uses it. I don't like having unused internal >>> functions and APIs sitting the source tree in the hope that someone will >>> find them useful in the future. It's more likely that it will bitrot, or >>> not actually work as intended, when someone later tries to use it. >> >> >> The thing is I already have extension for 9.4 where I would use this API >> for conflict detection if it was available so it's not about hoping for >> somebody to find it useful. There was discussion about this during the >> review process because the objection was raised already then. > > > Yeah, it was raised. I don't think it was really addressed. There was > lengthy discussion on whether to include LSN, node id, and/or some other > information, but there was no discussion on: > > * What is a node ID? > * How is it used? > * Who assigns it? > > etc. > > None of those things are explained in the documentation nor code comments. > > The node ID sounds suspiciously like the replication identifiers that have > been proposed a couple of times. I don't remember if I like replication > identifiers or not, but I'd rather discuss that issue explicitly and > separately. I don't want the concept of a replication/node ID to fly under > the radar like this. > >>> What would the SQL API look like, and what would it be used for? >> >> >> It would probably mirror the C one, from my POV it's not needed but >> maybe some replication solution would prefer to use this without having >> to write C components... > > > I can't comment on that, because without any documentation, I don't even > know what the C API is. > > BTW, why is it OK that the node-ID of a commit is logged as a separate WAL > record, after the commit record? That means that it's possible that a > transaction commits, but you crash just before writing the SETTS record, so > that after replay the transaction appears committed but with the default > node ID. > > (Maybe that's OK given the intended use case, but it's hard to tell without > any documentation. See a pattern here? ;-) )
Could it be possible to get a refresh on that before it bitrots too much or we'll simply forget about it? In the current state, there is no documentation able to explain what is the point of the nodeid stuff, how to use it and what use cases it is aimed at. The API in committs.c should as well be part of it. If that's not done, I think that it would be fair to remove this portion and simply WAL-log the commit timestamp its GUC is activated. Thanks, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
