Justin: For reindex_index() : + if (options->tablespaceOid == MyDatabaseTableSpace) + options->tablespaceOid = InvalidOid; ... + if (set_tablespace && + (options->tablespaceOid != oldTablespaceOid || + (options->tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid))))
I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause appears again in the second if statement. Since the first if statement would assign InvalidOid to options->tablespaceOid when the first if condition is satisfied. Cheers On Tue, Dec 22, 2020 at 1:15 PM Justin Pryzby <pry...@telsasoft.com> wrote: > On Tue, Dec 22, 2020 at 06:57:41PM +0900, Michael Paquier wrote: > > On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote: > > > Also, this one is going to be subsumed by ExecReindex(), so the palloc > will go > > > away (otherwise I would ask to pass it in from the caller): > > > > Yeah, maybe. Still you need to be very careful if you have any > > allocated variables like a tablespace or a path which requires to be > > in the private context used by ReindexMultipleInternal() or even > > ReindexRelationConcurrently(), so I am not sure you can avoid that > > completely. For now, we could choose the option to still use a > > palloc(), and then save the options in the private contexts. Forgot > > that in the previous version actually. > > I can't see why this still uses memset instead of structure assignment. > > Now, I really think utility.c ought to pass in a pointer to a local > ReindexOptions variable to avoid all the memory context, which is > unnecessary > and prone to error. > > ExecReindex() will set options.tablesapceOid, not a pointer. Like this. > > I also changed the callback to be a ReindexOptions and not a pointer. > > -- > Justin >
