Re: logical changeset generation v6.7

On 2013-12-04 17:31:50 +0900, Kyotaro HORIGUCHI wrote:
> ===== 0009:
>
> - In repl_scanner.l, you omitted double-doublequote handling for
> replication but it should be implemented. Zero-length
> identifier check might be needed depending on the upper-layer.

I am not sure what you mean here. IDENT can be double quoted, and so can
the option names?

> - In walsender.c, the log messages "Initiating logical rep.."
> and "Starting logical replication.." should be INFO or LOG in
> loglevel, not WARNING. And 'rep' in the former message would
> be better not abbreviated since not done so in the latter.

Agreed.

> - In walsender.c, StartLogicalReplication seems trying to abort
> itself for timeline change. But timeline changes in 9.3+ don't
> need such an aid. You'd better consult StartReplication in
> current master for detail. There might be other defferences.

Timeline increases currently need work, yes, that error messgage is the
smallest part...

> - In walsender.c, the typedef name WalSndSendData doesn't seem
> to be a function pointer. I suppose passing bare function
> pointer to WanSndLoop and WalSndDone is not a good deed. It'd
> be better to wrap it in any struct for callback, say,
> LogicalDecodingContext. It'd be even better if it could be a
> common struct with 'physycal' replication.

I don't see that as being realistic/advantageous. Wrapping a function
pointer in a struct doesn't improve anything in itself.

I was thinking we might want to just decouple the entire event loop and
not reuse that code, but that's ugly as well.

> - In walsender.c, I wonder if the differences are necessary
> between logical and physical replication in fetching latest
> WALs, construction of WAL sending loop and so on .. Logical
> walsender seems to be implimentated in somewhat ad-hoc way on
> the whole. I belive it could be more commonize in the base
> structure.

That's because the xlogreader.h interface - over my loud protests -
doesn't support chunk-wise reading of the WAL stream and neccessicates
blocking inside the reader callback. So the event loop needs to be in
several individual functions (WalSndLoop, WalSndWaitForWal,
WalSndWriteData) instead of once in WalSndLoop…

> - In procarray.c, the added two includes which is not
> accompanied by any other modification are needless. make emits
> no error or warning without them.

Right. Will remove.

Thanks,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services