Re: range_agg

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com>
Cc: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Jeff Davis <pgsql(at)j-davis(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: range_agg
Date: 2019-11-30 02:10:26
Message-ID: 20191130021026.GA19259@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Paul

I'm starting to look at this again. Here's a few proposed changes to
the current code as I read along.

I noticed that 0001 does not compile on its own. It works as soon as I
add 0002. What this is telling me is that the current patch split is
not serving any goals; I think it's okay to merge them all into a single
commit. If you want to split it in smaller pieces, it needs to be stuff
that can be committed separately.

One possible candidate for that is the new makeUniqueTypeName function
you propose. I added this comment to explain what it does:

- * makeUniqueTypeName: Prepend underscores as needed until we make a name that
- * doesn't collide with anything. Tries the original typeName if requested.
+ * makeUniqueTypeName
+ * Generate a unique name for a prospective new type
+ *
+ * Given a typeName of length namelen, produce a new name into dest (an output
+ * buffer allocated by caller, which must of length NAMEDATALEN) by prepending
+ * underscores, until a non-conflicting name results.
+ *
+ * If tryOriginalName, first try with zero underscores.
* Returns the number of underscores added.

This seems a little too strange; why not have the function allocate its
output buffer instead, and return it? In order to support the case of
it failing to find an appropriate name, have it return NULL, for caller
to throw the "could not form ... name" error.

The attached 0001 simplifies makeMultirangeConstructors; I think it was
too baroque just because of it trying to imitate makeRangeConstructors;
it seems easier to just repeat the ProcedureCreate call than trying to
be clever. (makeRangeConstructors's comment is lying about the number
of constructors it creates after commit df73584431e7, BTW. But note
that the cruft in it to support doing it twice is not as much as in the
new code).

The other patches should be self-explanatory (I already submitted 0002

I'll keep going over the rest of it. Thanks!

Álvaro Herrera
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment Content-Type Size
0001-Simplify-makeMultirangeConstructors.patch text/x-diff 6.2 KB
0002-silence-compiler-warning.patch text/x-diff 780 bytes
0003-Be-less-verbose-on-variable-names.patch text/x-diff 4.8 KB
0004-Protect-comment-against-pgindent.patch text/x-diff 1.3 KB
0005-pgindent.patch text/x-diff 3.5 KB
0006-Add-comment-and-asserts-to-makeUniqueTypeName.patch text/x-diff 1.5 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2019-11-30 02:16:16 Re: dropdb --force
Previous Message David Fetter 2019-11-29 22:21:53 Re: Make autovacuum sort tables in descending order of xid_age