MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_42e187ec7dedcb7c2ba7022b03b4a9e1"
Content-Transfer-Encoding: 7bit
Date: Fri, 04 Dec 2020 21:40:31 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>, Alvaro Herrera
 <alvherre@2ndquadrant.com>, Masahiko Sawada
 <masahiko.sawada@2ndquadrant.com>, Steve Singer <steve@ssinger.info>,
 pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>, Masahiko Sawada
 <sawada.mshk@gmail.com>, Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
In-Reply-To: <20201204012543.GW24052@telsasoft.com>
References: <20201031183611.GA22691@telsasoft.com>
 <20201124153123.GO24052@telsasoft.com> <X8TYkH2cJHzqFDGM@paquier.xyz>
 <bfc3903c0e30ef4349ec5cfee625d8d5@postgrespro.ru>
 <X8Wun1FBVgcqb6Fb@paquier.xyz> <20201201054308.GC24052@telsasoft.com>
 <X8XeRagOdYS3wCkT@paquier.xyz> <X8g9L7ZNymScmziJ@paquier.xyz>
 <20201203043008.GL24052@telsasoft.com> <X8iP9cFW8xdqmQsr@paquier.xyz>
 <20201204012543.GW24052@telsasoft.com>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <30bbac97f3ebbf1642e089583e4e45ba@postgrespro.ru>
Precedence: bulk

--=_42e187ec7dedcb7c2ba7022b03b4a9e1
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

On 2020-12-04 04:25, Justin Pryzby wrote:
> On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
>> > +typedef struct ReindexParams {
>> > +	bool concurrently;
>> > +	bool verbose;
>> > +	bool missingok;
>> > +
>> > +	int options;	/* bitmask of lowlevel REINDEXOPT_* */
>> > +} ReindexParams;
>> > +
>> 
>> By moving everything into indexcmds.c, keeping ReindexParams within it
>> makes sense to me.  Now, there is no need for the three booleans
>> because options stores the same information, no?
> 
>  I liked the bools, but dropped them so the patch is smaller.
> 

I had a look on 0001 and it looks mostly fine to me except some strange 
mixture of tabs/spaces in the ExecReindex(). There is also a couple of 
meaningful comments:

-	options =
-		(verbose ? REINDEXOPT_VERBOSE : 0) |
-		(concurrently ? REINDEXOPT_CONCURRENTLY : 0);
+	if (verbose)
+		params.options |= REINDEXOPT_VERBOSE;

Why do we need this intermediate 'verbose' variable here? We only use it 
once to set a bitmask. Maybe we can do it like this:

params.options |= defGetBoolean(opt) ?
	REINDEXOPT_VERBOSE : 0;

See also attached txt file with diff (I wonder can I trick cfbot this 
way, so it does not apply the diff).

+	int options;	/* bitmask of lowlevel REINDEXOPT_* */

I would prefer if the comment says '/* bitmask of ReindexOption */' as 
in the VacuumOptions, since citing the exact enum type make it easier to 
navigate source code.

> 
> Regarding the REINDEX patch, I think this comment is misleading:
> 
> |+                * Even if table was moved to new tablespace,
> normally toast cannot move.
> |                 */
> |+               Oid toasttablespaceOid = allowSystemTableMods ?
> tablespaceOid : InvalidOid;
> |                result |= reindex_relation(toast_relid, flags,
> 
> I think it ought to say "Even if a table's indexes were moved to a new
> tablespace, its toast table's index is not normally moved"
> Right ?
> 

Yes, I think so, we are dealing only with index tablespace changing 
here. Thanks for noticing.

> 
> Also, I don't know whether we should check for GLOBALTABLESPACE_OID 
> after
> calling get_tablespace_oid(), or in the lowlevel routines.  Note that
> reindex_relation is called during cluster/vacuum, and in the later 
> patches, I
> moved the test from from cluster() and ExecVacuum() to 
> rebuild_relation().
> 

IIRC, I wanted to do GLOBALTABLESPACE_OID check as early as possible 
(just after getting Oid), since it does not make sense to proceed 
further if tablespace is set to that value. So initially there were a 
lot of duplicative GLOBALTABLESPACE_OID checks, since there were a lot 
of reindex entry-points (index, relation, concurrently, etc.). Now we 
are going to have ExecReindex(), so there are much less entry-points and 
in my opinion it is fine to keep this validation just after 
get_tablespace_oid().

However, this is mostly a sanity check. I can hardly imagine a lot of 
users trying to constantly move indexes to the global tablespace, so it 
is also OK to put this check deeper into guts.


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company
--=_42e187ec7dedcb7c2ba7022b03b4a9e1
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=refactor-ExecReindex.txt
Content-Disposition: attachment;
 filename=refactor-ExecReindex.txt;
 size=1605

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL2luZGV4Y21kcy5jIGIvc3JjL2JhY2tl
bmQvY29tbWFuZHMvaW5kZXhjbWRzLmMKaW5kZXggYTI3ZjhmOWQ4My4uMGIxODg0ODE1YyAxMDA2
NDQKLS0tIGEvc3JjL2JhY2tlbmQvY29tbWFuZHMvaW5kZXhjbWRzLmMKKysrIGIvc3JjL2JhY2tl
bmQvY29tbWFuZHMvaW5kZXhjbWRzLmMKQEAgLTI0NzIsOCArMjQ3Miw2IEBAIHZvaWQKIEV4ZWNS
ZWluZGV4KFBhcnNlU3RhdGUgKnBzdGF0ZSwgUmVpbmRleFN0bXQgKnN0bXQsIGJvb2wgaXNUb3BM
ZXZlbCkKIHsKIAlSZWluZGV4UGFyYW1zCQlwYXJhbXMgPSB7MH07Ci0JYm9vbAkJdmVyYm9zZSA9
IGZhbHNlLAotCQkJCWNvbmN1cnJlbnRseSA9IGZhbHNlOwogCUxpc3RDZWxsICAgCSpsYzsKIAlj
aGFyCSp0YWJsZXNwYWNlID0gTlVMTDsKIApAQCAtMjQ4Myw5ICsyNDgxLDExIEBAIEV4ZWNSZWlu
ZGV4KFBhcnNlU3RhdGUgKnBzdGF0ZSwgUmVpbmRleFN0bXQgKnN0bXQsIGJvb2wgaXNUb3BMZXZl
bCkKIAkJRGVmRWxlbSAgICAqb3B0ID0gKERlZkVsZW0gKikgbGZpcnN0KGxjKTsKIAogCQlpZiAo
c3RyY21wKG9wdC0+ZGVmbmFtZSwgInZlcmJvc2UiKSA9PSAwKQotCQkJdmVyYm9zZSA9IGRlZkdl
dEJvb2xlYW4ob3B0KTsKKwkJCXBhcmFtcy5vcHRpb25zIHw9IGRlZkdldEJvb2xlYW4ob3B0KSA/
CisJCQkJUkVJTkRFWE9QVF9WRVJCT1NFIDogMDsKIAkJZWxzZSBpZiAoc3RyY21wKG9wdC0+ZGVm
bmFtZSwgImNvbmN1cnJlbnRseSIpID09IDApCi0JCQljb25jdXJyZW50bHkgPSBkZWZHZXRCb29s
ZWFuKG9wdCk7CisJCQlwYXJhbXMub3B0aW9ucyB8PSBkZWZHZXRCb29sZWFuKG9wdCkgPworCQkJ
CVJFSU5ERVhPUFRfQ09OQ1VSUkVOVExZIDogMDsKIAkJZWxzZSBpZiAoc3RyY21wKG9wdC0+ZGVm
bmFtZSwgInRhYmxlc3BhY2UiKSA9PSAwKQogCQkJdGFibGVzcGFjZSA9IGRlZkdldFN0cmluZyhv
cHQpOwogCQllbHNlCkBAIC0yNDk2LDE4ICsyNDk2LDEyIEBAIEV4ZWNSZWluZGV4KFBhcnNlU3Rh
dGUgKnBzdGF0ZSwgUmVpbmRleFN0bXQgKnN0bXQsIGJvb2wgaXNUb3BMZXZlbCkKIAkJCQkJIHBh
cnNlcl9lcnJwb3NpdGlvbihwc3RhdGUsIG9wdC0+bG9jYXRpb24pKSk7CiAJfQogCi0JaWYgKHZl
cmJvc2UpCi0JCXBhcmFtcy5vcHRpb25zIHw9IFJFSU5ERVhPUFRfVkVSQk9TRTsKKwlwYXJhbXMu
dGFibGVzcGFjZU9pZCA9IHRhYmxlc3BhY2UgPworCQlnZXRfdGFibGVzcGFjZV9vaWQodGFibGVz
cGFjZSwgZmFsc2UpIDogSW52YWxpZE9pZDsKIAotCWlmIChjb25jdXJyZW50bHkpCi0JewotCQlw
YXJhbXMub3B0aW9ucyB8PSBSRUlOREVYT1BUX0NPTkNVUlJFTlRMWTsKKwlpZiAocGFyYW1zLm9w
dGlvbnMgJiBSRUlOREVYT1BUX0NPTkNVUlJFTlRMWSkKIAkJUHJldmVudEluVHJhbnNhY3Rpb25C
bG9jayhpc1RvcExldmVsLAogCQkJCQkJCQkgICJSRUlOREVYIENPTkNVUlJFTlRMWSIpOwotCX0K
LQotCXBhcmFtcy50YWJsZXNwYWNlT2lkID0gdGFibGVzcGFjZSA/Ci0JCWdldF90YWJsZXNwYWNl
X29pZCh0YWJsZXNwYWNlLCBmYWxzZSkgOiBJbnZhbGlkT2lkOwogCiAJc3dpdGNoIChzdG10LT5r
aW5kKQogCXsK
--=_42e187ec7dedcb7c2ba7022b03b4a9e1--