On Wed, Sep 18, 2019 at 10:33 PM Fabien COELHO <coe...@cri.ensmp.fr> wrote:
>
>
> Hello Amit,
>
> >> - use search_path to find at most one pgbench_accounts
> >> It still uses left join because I still think that it is appropriate.
> >> I added a lateral to avoid repeating the array_position call
> >> to manage the search_path, and use explicit pg_catalog everywhere.
> >
> > It would be good if you can add some more comments to explain the
> > intent of query.
>
> Indeed, I put too few comments on the query.
>
> > + if (ps == NULL)
> > + partition_method = PART_NONE;
> >
> > When can we expect ps as NULL? If this is not a valid case, then
> > probably and Assert would be better.
>
> No, ps is really NULL if there is no partitioning, because of the LEFT
> JOIN and pg_partitioned_table is just empty in that case.
>
'ps' itself won't be NULL in that case, the value it contains is NULL.
I have debugged this case as well. 'ps' itself can be NULL only when
you pass wrong column number or something like that to PQgetvalue.
> The last else where there is an unexpected entry is different, see
> comments about v11 below.
>
> > + else if (PQntuples(res) == 0)
> > + {
> > + /* no pgbench_accounts found, builtin script should fail later */
> > + partition_method = PART_NONE;
> > + partitions = -1;
> >
> > If we don't find pgbench_accounts, let's give error here itself rather
> > than later unless you have a valid case in mind.
>
> I thought of it, but decided not to: Someone could add a builtin script
> which does not use pgbench_accounts, or a parallel running script could
> create a table dynamically, whatever, so I prefer the error to be raised
> by the script itself, rather than deciding that it will fail before even
> trying.
>
I think this is not a possibility today and I don't know of the
future. I don't think it is a good idea to add code which we can't
reach today. You can probably add Assert if required.
> > + /*
> > + * Partition information. Assume no partitioning on any failure, so as
> > + * to avoid failing on an older version.
> > + */
> > ..
> > + if (PQresultStatus(res) != PGRES_TUPLES_OK)
> > + {
> > + /* probably an older version, coldly assume no partitioning */
> > + partition_method = PART_NONE;
> > + partitions = 0;
> > + }
> >
> > So, here we are silently absorbing the error when pgbench is executed
> > against older server version which doesn't support partitioning.
>
> Yes, exactly.
>
> > If that is the case, then I think if user gives --partitions for the old
> > server version, it will also give an error?
>
> Yes, on -i it will fail because the syntax will not be recognized.
>
> > It is not clear in documentation whether we support or not using pgbench
> > with older server versions.
>
> Indeed. We more or less do in practice. Command "psql" works back to 8
> AFAICR, and pgbench as well.
>
> > I guess it didn't matter, but with this feature, it can matter. Do we
> > need to document this?
>
> This has been discussed in the past, and the conclusion was that it was
> not worth the effort. We just try not to break things if it is avoidable.
> On this regard, the patch slightly changes FILLFACTOR output, which is
> removed if the value is 100 (%) as it is the default, which means that
> table creation would work on very very old version which did not support
> fillfactor, unless you specify a lower percentage.
>
Hmm, why you need to change the fill factor behavior? If it is not
specifically required for the functionality of this patch, then I
suggest keeping that behavior as it is.
> Attached v11:
>
> - add quite a few comments on the pg_catalog query
>
> - reverts the partitions >= 1 test; If some new partition method is
> added that pgbench does not know about, the failure mode will be that
> nothing is printed rather than printing something strange like
> "method none with 2 partitions".
>
but how will that new partition method will be associated with a table
created via pgbench? I think the previous check was good because it
makes partition checking consistent throughout the patch.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
