On 2020-04-03 21:27, Justin Pryzby wrote:
On Wed, Apr 01, 2020 at 08:08:36AM -0500, Justin Pryzby wrote:
Or maybe you'd want me to squish my changes into yours and resend
after any
review comments ?
I didn't hear any feedback, so I've now squished all "parenthesized"
and "fix"
commits.
Thanks for the input, but I am afraid that the patch set became a bit
messy now. I have eyeballed it and found some inconsistencies.
const char *name; /* name of database to reindex
*/
- int options; /* Reindex options
flags */
+ List *rawoptions; /* Raw options */
+ int options; /* Parsed options */
bool concurrent; /* reindex concurrently? */
You introduced rawoptions in the 0002, but then removed it in 0003. So
is it required or not? Probably this is a rebase artefact.
+/* XXX: reusing reindex_option_list */
+ | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name
cluster_index_specification
Could we actually simply reuse vac_analyze_option_list? From the first
sight it does just the right thing, excepting the special handling of
spelling ANALYZE/ANALYSE, but it does not seem to be a problem.
0004 reduces duplicative error handling, as a separate commit so
Alexey can review it and/or integrate it.
@@ -2974,27 +2947,6 @@ ReindexRelationConcurrently(Oid relationOid, Oid
tablespaceOid, int options)
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
- /*
- * We don't support moving system relations into different
tablespaces,
- * unless allow_system_table_mods=1.
- */
- if (OidIsValid(tablespaceOid) &&
- !allowSystemTableMods && IsSystemRelation(heapRelation))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system
catalog",
-
RelationGetRelationName(heapRelation))));
ReindexRelationConcurrently is used for all cases, but it hits different
code paths in the case of database, table and index. I have not checked
yet, but are you sure it is safe removing these validations in the case
of REINDEX CONCURRENTLY?
The last two commits save a few
dozen lines of code, but not sure they're desirable.
Sincerely, I do not think that passing raw strings down to the guts is a
good idea. Yes, it saves us a few checks here and there now, but it may
reduce a further reusability of these internal routines in the future.
XXX: for cluster/vacuum, it might be more friendly to check before
clustering
the table, rather than after clustering and re-indexing.
Yes, I think it would be much more user-friendly.
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
