From: Alvaro Herrera on
Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
> >Andrew Dunstan <andrew(a)dunslane.net> writes:
> >>Jeff Davis wrote:
> >>>So, I think ASCII is the natural choice here.
> >
> >>It's not worth hanging up this facility over this issue, ISTM.
> >>If we want something more that ASCII then a base64 or hex
> >>encoded string could possibly meet the need in the first
> >>instance.
> >
> >Yeah, that would serve people who want to push either binary or
> >non-ASCII data through the pipe. It would leave all risks of encoding
> >problems on the user's head, though.
>
> True. It's a workaround, but I think it's acceptable at this stage.
> We need to get some experience with this facility before we can
> refine it.

Hmm? If we decide now that it's not going to have encoding conversion,
we won't able to change it later.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on
Alvaro Herrera <alvherre(a)commandprompt.com> writes:
> Andrew Dunstan wrote:
>> True. It's a workaround, but I think it's acceptable at this stage.
>> We need to get some experience with this facility before we can
>> refine it.

> Hmm? If we decide now that it's not going to have encoding conversion,
> we won't able to change it later.

How so? If the feature currently allows only ASCII data, the behavior
would be upward compatible with a future version that is able to accept
and convert non-ASCII characters.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Simon Riggs on
On Thu, 2010-02-11 at 00:52 +0100, Joachim Wieland wrote:
> On Mon, Feb 8, 2010 at 5:16 PM, Alvaro Herrera
> <alvherre(a)commandprompt.com> wrote:
> > These are the on-disk notifications, right? It seems to me a bit
> > wasteful to store channel name always as NAMEDATALEN bytes. Can we
> > truncate it at its strlen?
>
> Attached is a new and hopefully more or less final patch for LISTEN / NOTIFY.
>
> The following items have been addressed in this patch:
>
> - only store strlen(channel) instead of NAMEDATALEN bytes on disk
> - limit to 7-bit ASCII
> - forbid 2PC and LISTEN/NOTIFY for now
> - documentation changes
> - add missing tab completion for NOTIFY
> - fix pg_notify() behavior with respect to NULLs, too long and too
> short parameters
> - rebased to current HEAD, OID conflicts resolved


Some minor review comments without having taken in much of previous
discussion:

* Patch doesn't apply cleanly anymore, close.

* In async.c you say "new async notification model". Please say "async
notification model in 9.0 is". In (2) you say there is a central queue,
but don't describe purpose of queue or what it contains. Some other
typos in header comments.

* There is no mention of what to do with pg_notify at checkpoint. Look
at how pg_subtrans handles this. Should pg_notify do the same?

* Is there a lazy backend avoidance scheme as exists for relcache
invalidation messages? see storage/ipc/sinval...
OK, I see asyncQueueFillWarning() but nowhere else says it exists and
there aren't any comments in it to say what it does or why

* What do you expect the DBA to do when they receive a queue fill
warning? Where is that written down?

* Not clear of the purpose of backendSendsNotifications. In
AtCommit_NotifyAfterCommit() the logic seems strange. Code is
/* Allow transactions that have not executed
LISTEN/UNLISTEN/NOTIFY to
* return as soon as possible */
if (!pendingActions && !backendSendsNotifications)
return;
but since backendSendsNotifications is set true earlier there is no fast
path.

* AtSubCommit_Notify doesn't seem to have a fastpath when no notify
commands have been executed.

* In Send_Notify() you throw ERROR while still holding the lock. It
seems better practice to drop the lock then ERROR.

* Why is Send_Notify() a separate function? It's only called from one
point in the code.

* We know that sometimes a FATAL error can occur without properly
processing the abort. Do we depend upon this never happening?

* Can we make NUM_ASYNC_BUFFERS = 8? 4 just seems too small

* backendSendsNotifications is future tense. The variable should be
called something like hasSentNotifications. Couple of other variables
with similar names

Hope that helps

--
Simon Riggs www.2ndQuadrant.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Simon Riggs on
On Sun, 2010-02-14 at 17:22 +0100, Joachim Wieland wrote:
> > * There is no mention of what to do with pg_notify at checkpoint.
> Look
> > at how pg_subtrans handles this. Should pg_notify do the same?
>
> Actually we don't care... We even hope that the pg_notify pages are
> not flushed at all. Notifications don't survive a server restart
> anyway and upon restart we just delete whatever is in the directory.

Suspected that was true, just checking it was commented somewhere.

--
Simon Riggs www.2ndQuadrant.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on
Joachim Wieland <joe(a)mcknight.de> writes:
> + #define ERRCODE_TOO_MANY_ENTRIES MAKE_SQLSTATE('5','4', '0','3','1')

Do you have any evidence that there is actually a DB2 error code
matching this, or is this errcode just invented? The one page
Google finds doesn't list it:
http://publib.boulder.ibm.com/iseries/v5r1/ic2924/index.htm?info/rzala/rzalastc.html

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers