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 |
Thread: | |
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
previously.)
I'll keep going over the rest of it. Thanks!
--
Álvaro Herrera https://www.2ndQuadrant.com/
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 |
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 |