Re: Why is pq_begintypsend so slow?

Here is some review for the first few patches in this series.

I am generally in favor of applying 0001-0003 no matter what else we
decide to do here. However, as might be expected, I have a few
questions and comments.

Regarding 0001:

I dislike the name initStringInfoEx() because we do not use the "Ex"
convention for function names anywhere in the code base. We do
sometimes use "extended", so this could be initStringInfoExtended(),
perhaps, or something more specific, like initStringInfoWithLength().

Regarding the FIXME in that function, I suggest that this should be
the caller's job. Otherwise, there will probably be some caller which
doesn't want the add-one behavior, and then said caller will subtract
one to compensate, and that will be silly.

I am not familiar with pg_restrict and don't entirely understand the
motivation for it here. I suspect you have done some experiments and
figured out that it produces better code, but it would be interesting
to hear more about how you got there. Perhaps there could even be some
brief comments about it. Locutions like this are particularly
confusing to me:

+static inline void
+resetStringInfo(StringInfoData *pg_restrict str)
+{
+       *(char *pg_restrict) (str->data) = '\0';
+       str->len = 0;
+       str->cursor = 0;
+}

I don't understand how this can be saving anything. I think the
restrict definitions here mean that str->data does not overlap with
str itself, but considering that these are unconditional stores, so
what? If the code were doing something like memset(str->data, 0,
str->len) then I'd get it: it might be useful to know for optimization
purposes that the memset isn't overwriting str->len. But what code can
we produce for this that wouldn't be valid if str->data = &str? I
assume this is my lack of understanding rather than an actual problem
with the patch, but I would be grateful if you could explain.

It is easier to see why the pg_restrict stuff you've introduced into
appendBinaryStringInfoNT is potentially helpful: e.g. in
appendBinaryStringInfoNT, it promises that memcpy can't clobber
str->len, so the compiler is free to reorder without changing the
results. Or so I imagine. But then the one in appendBinaryStringInfo()
confuses me again: if str->data is already declared as a restricted
pointer, then why do we need to cast str->data + str->len to be
restricted also?

In appendStringInfoChar, why do we need to cast to restrict twice? Can
we not just do something like this:

char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';

Regarding 0002:

Totally mechanical, seems fine.

Regarding 0003:

For the same reasons as above, I suggest renaming pq_begintypsend_ex()
to pq_begintypsend_extended() or pq_begintypsend_with_length() or
something of that sort, rather than using _ex.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company