Date: Wed, 25 Sep 2019 11:15:18 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nikolay Shaplov <dhyan@nataraj.su>
Cc: Thomas Munro <thomas.munro@gmail.com>, Dent John <denty@qqdd.eu>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>
Subject: Re: [PATCH] src/test/modules/dummy_index -- way to test reloptions
 from inside of access method
Message-ID: <20190925021518.GC1815@paquier.xyz>
References: <17071942.m9zZutALE6@x200m>
 <CA+hUKG+8294zbN_k=QDgcW2TFia5TqsTR8nJLyYU+_C8vUTmrg@mail.gmail.com>
 <20190919083203.GC21144@paquier.xyz>
 <6059120.LlfbHtIvrV@x200m>
 <20190920001658.GA1844@paquier.xyz>
 <20190920115827.GC18018@paquier.xyz>
 <9629433.XHBmnWSU8E@x200m>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="uh9ZiVrAOUUm9fzH"
Content-Disposition: inline
In-Reply-To: <9629433.XHBmnWSU8E@x200m>
Precedence: bulk


--uh9ZiVrAOUUm9fzH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Sep 24, 2019 at 04:49:11PM +0300, Nikolay Shaplov wrote:
> And then we came to a GUC variables. Because it we have five reloptions
> and we print them all each time we change something, there would be
> quite huge output.

Well, that depends on how you design your tests.  The first versions
of the patch overdid it, and those GUCs have IMO little place for a
module aimed as well to be a minimized referential template focused on
testing some portions of the backend code.

> It is ok when everything goes well. Comparing with 'expected' is cheap.
> But is something goes wrong, then it would be very difficult to find
> proper place in this output to deal with it.
> So I created GUCs so we can get only one output in a row, not a whole
> bunch.

I am still not convinced that this is worth the complication.  Your
point is that you want to make *on-demand* and *user-visible* the set
of options stored in rd_options after filling in the relation options
using the static table used in the AM.

One way to do that could be to have a simple wrapper function which
could be called at SQL level to do those checks, or you could issue a
NOTICE with all the data filled in amoptions() or even ambuild(),
though the former makes the most sense as we fill in the options
there.

One thing that I think would value in the module would be to show how
a custom string option can be properly parsed when doing some
decisions in the AM.  Now we store an offset in the static table, and
one needs to do a small dance with it to fetch the actual option
value.

This can be guessed easily as for example gist has a string option
with "buffering", but we could document that better in the dummy
template, say like that:
@@ -206,6 +210,15 @@ dioptions(Datum reloptions, bool validate)
    fillRelOptions((void *) rdopts, sizeof(DummyIndexOptions), options, numoptions,
                   validate, di_relopt_tab,  lengthof(di_relopt_tab));

+   option_string_val = (char *) rdopts + rdopts->option_string_val_offset;
+   option_string_null = (char *) rdopts + rdopts->option_string_null_offset;
+   ereport(NOTICE,
+           (errmsg("table option_int %d, option_real %f, option_bool %s, "
+                   "option_string_val %s, option_option_null %s",
+                   rdopts->option_int, rdopts->option_real,
+                   rdopts->option_bool ? "true" : "false",
+                   option_string_val ? option_string_val : "NULL",
+                   option_string_null ? option_string_null : "NULL")));

The patch I have in my hands now is already doing a lot, so I am
discarding that part for now.  And we can easily improve it
incrementally.

(One extra thing which is also annoying with the current interface is
that we don't actually pass down the option name within the validator
function for string options like GUCs, so you cannot know on which
option you work on if a module generates logs, I'll send an extra
patch for that on a separate thread.)
--
Michael

--uh9ZiVrAOUUm9fzH
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2KzbYACgkQnvQgOdby
QH1AwA/7BjKErwEf1UHvRiVS41N494Rn5Jb7pGtxJ+I4OVD+5UbZPyblmbKS4j8J
JINSlJQnm0AqSkq8tfZPI3BG9gVRreZoon6JILT03tszUNHuf63tD+oQIK7He0b6
oOwPyRdnLoUCmTzVB4LBB4p6LWZFfxaAZicb9c2Nzs7Sydi1LXODIta/Z+SB/3U5
W6/EgcPTTIIBKAwMtmxN5mQQ8iBHp1Xky65Na25FGJx3+76trOC9ycfUR5zumlQN
51rXZsI2npjZ3MYrRAqexw+LiyXmsfEkGI0qK60Fs4WNZkZz19pkFNLTtxCmZfdO
+HJkSUhj8Leg9UjzJ2Qa8AfbkjAF7bc2Kobd8mB/0Zse5fXK8iKODuAVtAVFiynf
UDw6Fl9l96ym8qQUhfc/HOyIEkTE5nY8PzVsdhItzWrhb9L1G0NWJAOTiH3xrQAo
O2iyL6AxjbXj+yPJEI8O5hohjSIFZCJn+7PPzmBSdSYSqlmJoalimYhDdJS2voC2
hVcv6j6SEy0RCIUHRoL9DiHEtc10ADdEmnHzWlYVTSuHFMPKWgOVJjN6yjA9vHUi
LHY/EkJGREpxVOc1yTNBkkuOVCS56wYzvB60jsdOpvxFFqhubjXkw0jXUPTe+XoQ
60HBliRv7/K0D4u9+juyx8HJ/BnKPjTldNprXTTtFMJhdFVq8Mk=
=Fz/I
-----END PGP SIGNATURE-----

--uh9ZiVrAOUUm9fzH--