public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/8] Move processing of NULLs from BRIN support functions
7+ messages / 4 participants
[nested] [flat]
* [PATCH 3/8] Move processing of NULLs from BRIN support functions
@ 2020-04-02 00:56 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Tomas Vondra @ 2020-04-02 00:56 UTC (permalink / raw)
---
src/backend/access/brin/brin.c | 260 ++++++++++++++---------
src/backend/access/brin/brin_inclusion.c | 44 +---
src/backend/access/brin/brin_minmax.c | 41 +---
src/include/access/brin_internal.h | 3 +
4 files changed, 169 insertions(+), 179 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index caf7b62688..a9c44c0b82 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -35,6 +35,7 @@
#include "storage/freespace.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/datum.h"
#include "utils/index_selfuncs.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -77,7 +78,9 @@ static void form_and_insert_tuple(BrinBuildState *state);
static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a,
BrinTuple *b);
static void brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy);
-
+static bool add_values_to_range(Relation idxRel, BrinDesc *bdesc,
+ BrinMemTuple *dtup, Datum *values, bool *nulls);
+static bool check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys);
/*
* BRIN handler function: return IndexAmRoutine with access method parameters
@@ -178,7 +181,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
OffsetNumber off;
BrinTuple *brtup;
BrinMemTuple *dtup;
- int keyno;
CHECK_FOR_INTERRUPTS();
@@ -242,31 +244,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
dtup = brin_deform_tuple(bdesc, brtup, NULL);
- /*
- * Compare the key values of the new tuple to the stored index values;
- * our deformed tuple will get updated if the new tuple doesn't fit
- * the original range (note this means we can't break out of the loop
- * early). Make a note of whether this happens, so that we know to
- * insert the modified tuple later.
- */
- for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
- {
- Datum result;
- BrinValues *bval;
- FmgrInfo *addValue;
-
- bval = &dtup->bt_columns[keyno];
- addValue = index_getprocinfo(idxRel, keyno + 1,
- BRIN_PROCNUM_ADDVALUE);
- result = FunctionCall4Coll(addValue,
- idxRel->rd_indcollation[keyno],
- PointerGetDatum(bdesc),
- PointerGetDatum(bval),
- values[keyno],
- nulls[keyno]);
- /* if that returned true, we need to insert the updated tuple */
- need_insert |= DatumGetBool(result);
- }
+ need_insert = add_values_to_range(idxRel, bdesc, dtup, values, nulls);
if (!need_insert)
{
@@ -359,6 +337,7 @@ brinbeginscan(Relation r, int nkeys, int norderbys)
return scan;
}
+
/*
* Execute the index scan.
*
@@ -562,69 +541,31 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
(nkeys[attno - 1] <= scan->numberOfKeys));
/*
- * First check if there are any IS [NOT] NULL scan keys, and
- * if we're violating them. In that case we can terminate
- * early, without invoking the support function.
+ * First check if there are any IS [NOT] NULL scan keys,
+ * and if we're violating them. In that case we can
+ * terminate early, without invoking the support function.
*
* As there may be more keys, we can only detemine mismatch
* within this loop.
*/
- for (keyno = 0; (keyno < nnullkeys[attno - 1]); keyno++)
+ if (bdesc->bd_info[attno - 1]->oi_regular_nulls &&
+ !check_null_keys(bval, nullkeys[attno - 1],
+ nnullkeys[attno - 1]))
{
- ScanKey key = nullkeys[attno - 1][keyno];
-
- Assert(key->sk_attno == bval->bv_attno);
-
- /* interrupt the loop as soon as we find a mismatch */
- if (!addrange)
- break;
-
- /* handle IS NULL/IS NOT NULL tests */
- if (key->sk_flags & SK_ISNULL)
- {
- /* IS NULL scan key, but range has no NULLs */
- if (key->sk_flags & SK_SEARCHNULL)
- {
- if (!bval->bv_allnulls && !bval->bv_hasnulls)
- addrange = false;
-
- continue;
- }
-
- /*
- * For IS NOT NULL, we can only skip ranges that are
- * known to have only nulls.
- */
- if (key->sk_flags & SK_SEARCHNOTNULL)
- {
- if (bval->bv_allnulls)
- addrange = false;
-
- continue;
- }
-
- /*
- * Neither IS NULL nor IS NOT NULL was used; assume all
- * indexable operators are strict and thus return false
- * with NULL value in the scan key.
- */
- addrange = false;
- }
- }
-
- /*
- * If any of the IS [NOT] NULL keys failed, the page range as
- * a whole can't pass. So terminate the loop.
- */
- if (!addrange)
+ /*
+ * If any of the IS [NOT] NULL keys failed, the page
+ * range as a whole can't pass. So terminate the loop.
+ */
+ addrange = false;
break;
+ }
/*
- * So either there are no IS [NOT] NULL keys, or all passed. If
- * there are no regular scan keys, we're done - the page range
- * matches. If there are regular keys, but the page range is
- * marked as 'all nulls' it can't possibly pass (we're assuming
- * the operators are strict).
+ * So either there are no IS [NOT] NULL keys, or all passed.
+ * If there are no regular scan keys, we're done - the page
+ * range matches. If there are regular keys, but the page
+ * range is marked as 'all nulls' it can't possibly pass
+ * (we're assuming the operators are strict).
*/
/* No regular scan keys - page range as a whole passes. */
@@ -772,7 +713,6 @@ brinbuildCallback(Relation index,
{
BrinBuildState *state = (BrinBuildState *) brstate;
BlockNumber thisblock;
- int i;
thisblock = ItemPointerGetBlockNumber(tid);
@@ -801,25 +741,8 @@ brinbuildCallback(Relation index,
}
/* Accumulate the current tuple into the running state */
- for (i = 0; i < state->bs_bdesc->bd_tupdesc->natts; i++)
- {
- FmgrInfo *addValue;
- BrinValues *col;
- Form_pg_attribute attr = TupleDescAttr(state->bs_bdesc->bd_tupdesc, i);
-
- col = &state->bs_dtuple->bt_columns[i];
- addValue = index_getprocinfo(index, i + 1,
- BRIN_PROCNUM_ADDVALUE);
-
- /*
- * Update dtuple state, if and as necessary.
- */
- FunctionCall4Coll(addValue,
- attr->attcollation,
- PointerGetDatum(state->bs_bdesc),
- PointerGetDatum(col),
- values[i], isnull[i]);
- }
+ (void) add_values_to_range(index, state->bs_bdesc, state->bs_dtuple,
+ values, isnull);
}
/*
@@ -1598,6 +1521,39 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
FmgrInfo *unionFn;
BrinValues *col_a = &a->bt_columns[keyno];
BrinValues *col_b = &db->bt_columns[keyno];
+ BrinOpcInfo *opcinfo = bdesc->bd_info[keyno];
+
+ if (opcinfo->oi_regular_nulls)
+ {
+ /* Adjust "hasnulls". */
+ if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
+ col_a->bv_hasnulls = true;
+
+ /* If there are no values in B, there's nothing left to do. */
+ if (col_b->bv_allnulls)
+ continue;
+
+ /*
+ * Adjust "allnulls". If A doesn't have values, just copy the
+ * values from B into A, and we're done. We cannot run the
+ * operators in this case, because values in A might contain
+ * garbage. Note we already established that B contains values.
+ */
+ if (col_a->bv_allnulls)
+ {
+ int i;
+
+ col_a->bv_allnulls = false;
+
+ for (i = 0; i < opcinfo->oi_nstored; i++)
+ col_a->bv_values[i] =
+ datumCopy(col_b->bv_values[i],
+ opcinfo->oi_typcache[i]->typbyval,
+ opcinfo->oi_typcache[i]->typlen);
+
+ continue;
+ }
+ }
unionFn = index_getprocinfo(bdesc->bd_index, keyno + 1,
BRIN_PROCNUM_UNION);
@@ -1651,3 +1607,103 @@ brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy)
*/
FreeSpaceMapVacuum(idxrel);
}
+
+static bool
+add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup,
+ Datum *values, bool *nulls)
+{
+ int keyno;
+ bool modified = false;
+
+ /*
+ * Compare the key values of the new tuple to the stored index values;
+ * our deformed tuple will get updated if the new tuple doesn't fit
+ * the original range (note this means we can't break out of the loop
+ * early). Make a note of whether this happens, so that we know to
+ * insert the modified tuple later.
+ */
+ for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
+ {
+ Datum result;
+ BrinValues *bval;
+ FmgrInfo *addValue;
+
+ bval = &dtup->bt_columns[keyno];
+
+ if (bdesc->bd_info[keyno]->oi_regular_nulls && nulls[keyno])
+ {
+ /*
+ * If the new value is null, we record that we saw it if it's
+ * the first one; otherwise, there's nothing to do.
+ */
+ if (!bval->bv_hasnulls)
+ {
+ bval->bv_hasnulls = true;
+ modified = true;
+ }
+
+ continue;
+ }
+
+ addValue = index_getprocinfo(idxRel, keyno + 1,
+ BRIN_PROCNUM_ADDVALUE);
+ result = FunctionCall4Coll(addValue,
+ idxRel->rd_indcollation[keyno],
+ PointerGetDatum(bdesc),
+ PointerGetDatum(bval),
+ values[keyno],
+ nulls[keyno]);
+ /* if that returned true, we need to insert the updated tuple */
+ modified |= DatumGetBool(result);
+ }
+
+ return modified;
+}
+
+static bool
+check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys)
+{
+ int keyno;
+
+ /*
+ * First check if there are any IS [NOT] NULL scan keys, and if we're
+ * violating them.
+ */
+ for (keyno = 0; keyno < nnullkeys; keyno++)
+ {
+ ScanKey key = nullkeys[keyno];
+
+ Assert(key->sk_attno == bval->bv_attno);
+
+ /* Handle only IS NULL/IS NOT NULL tests */
+ if (!(key->sk_flags & SK_ISNULL))
+ continue;
+
+ if (key->sk_flags & SK_SEARCHNULL)
+ {
+ /* IS NULL scan key, but range has no NULLs */
+ if (!bval->bv_allnulls && !bval->bv_hasnulls)
+ return false;
+ }
+ else if (key->sk_flags & SK_SEARCHNOTNULL)
+ {
+ /*
+ * For IS NOT NULL, we can only skip ranges that are known to
+ * have only nulls.
+ */
+ if (bval->bv_allnulls)
+ return false;
+ }
+ else
+ {
+ /*
+ * Neither IS NULL nor IS NOT NULL was used; assume all indexable
+ * operators are strict and thus return false with NULL value in
+ * the scan key.
+ */
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 22edc6b46f..59503b6f68 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -110,6 +110,7 @@ brin_inclusion_opcinfo(PG_FUNCTION_ARGS)
*/
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(3)) + sizeof(InclusionOpaque));
result->oi_nstored = 3;
+ result->oi_regular_nulls = true;
result->oi_opaque = (InclusionOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(3));
@@ -141,7 +142,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
Datum newval = PG_GETARG_DATUM(2);
- bool isnull = PG_GETARG_BOOL(3);
+ bool isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_BOOL(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *finfo;
Datum result;
@@ -149,18 +150,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
AttrNumber attno;
Form_pg_attribute attr;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -510,37 +500,11 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
Datum result;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls". */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do. */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[INCLUSION_UNION] =
- datumCopy(col_b->bv_values[INCLUSION_UNION],
- attr->attbyval, attr->attlen);
- col_a->bv_values[INCLUSION_UNMERGEABLE] =
- col_b->bv_values[INCLUSION_UNMERGEABLE];
- col_a->bv_values[INCLUSION_CONTAINS_EMPTY] =
- col_b->bv_values[INCLUSION_CONTAINS_EMPTY];
- PG_RETURN_VOID();
- }
-
/* If B includes empty elements, mark A similarly, if needed. */
if (!DatumGetBool(col_a->bv_values[INCLUSION_CONTAINS_EMPTY]) &&
DatumGetBool(col_b->bv_values[INCLUSION_CONTAINS_EMPTY]))
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 7a7bd21cec..8882eec12c 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -48,6 +48,7 @@ brin_minmax_opcinfo(PG_FUNCTION_ARGS)
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(2)) +
sizeof(MinmaxOpaque));
result->oi_nstored = 2;
+ result->oi_regular_nulls = true;
result->oi_opaque = (MinmaxOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(2));
result->oi_typcache[0] = result->oi_typcache[1] =
@@ -69,7 +70,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
Datum newval = PG_GETARG_DATUM(2);
- bool isnull = PG_GETARG_DATUM(3);
+ bool isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_DATUM(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *cmpFn;
Datum compar;
@@ -77,18 +78,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
Form_pg_attribute attr;
AttrNumber attno;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -245,34 +235,11 @@ brin_minmax_union(PG_FUNCTION_ARGS)
bool needsadj;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls" */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[0] = datumCopy(col_b->bv_values[0],
- attr->attbyval, attr->attlen);
- col_a->bv_values[1] = datumCopy(col_b->bv_values[1],
- attr->attbyval, attr->attlen);
- PG_RETURN_VOID();
- }
-
/* Adjust minimum, if B's min is less than A's min */
finfo = minmax_get_strategy_procinfo(bdesc, attno, attr->atttypid,
BTLessStrategyNumber);
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 9ffc9100c0..7c4f3da0a0 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -27,6 +27,9 @@ typedef struct BrinOpcInfo
/* Number of columns stored in an index column of this opclass */
uint16 oi_nstored;
+ /* Regular processing of NULLs in BrinValues? */
+ bool oi_regular_nulls;
+
/* Opaque pointer for the opclass' private use */
void *oi_opaque;
--
2.25.4
--jsivyprvf2oxfjz3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="0004-BRIN-bloom-indexes-20200807.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: meson oddities
@ 2023-01-04 11:35 Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2023-01-04 11:35 UTC (permalink / raw)
To: Andres Freund <[email protected]>; Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
On 16.11.22 18:07, Andres Freund wrote:
>>> If I just want to install postgres into a prefix without 'postgresql' added in
>>> a bunch of directories, e.g. because I already have pg-$version to be in the
>>> prefix, there's really no good way to do so - you can't even specify
>>> --sysconfdir or such, because we just override that path.
>>
>> At least for the libraries, the point of the 'postgresql' subdir IMO
>> is to keep backend-loadable extensions separate from random libraries.
>> It's not great that we may fail to do that depending on what the
>> initial part of the library path is.
>
> Agreed, extensions really should never be in a path searched by the dynamic
> linker, even if the prefix contains 'postgres'.
>
> To me that's a separate thing from adding postgresql to datadir, sysconfdir,
> includedir, docdir... On a green field I'd say the 'extension library'
> directory should just always be extensions/ or such.
I think we should get the two build systems to produce the same
installation layout when given equivalent options.
Unless someone comes up with a proposal to address the above broader
issues, also taking into account current packaging practices etc., then
I think we should do a short-term solution to either port the
subdir-appending to the meson scripts or remove it from the makefiles
(or maybe a bit of both).
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: meson oddities
@ 2023-01-04 19:35 Andres Freund <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Andres Freund @ 2023-01-04 19:35 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2023-01-04 12:35:35 +0100, Peter Eisentraut wrote:
> On 16.11.22 18:07, Andres Freund wrote:
> > > > If I just want to install postgres into a prefix without 'postgresql' added in
> > > > a bunch of directories, e.g. because I already have pg-$version to be in the
> > > > prefix, there's really no good way to do so - you can't even specify
> > > > --sysconfdir or such, because we just override that path.
> > >
> > > At least for the libraries, the point of the 'postgresql' subdir IMO
> > > is to keep backend-loadable extensions separate from random libraries.
> > > It's not great that we may fail to do that depending on what the
> > > initial part of the library path is.
> >
> > Agreed, extensions really should never be in a path searched by the dynamic
> > linker, even if the prefix contains 'postgres'.
> >
> > To me that's a separate thing from adding postgresql to datadir, sysconfdir,
> > includedir, docdir... On a green field I'd say the 'extension library'
> > directory should just always be extensions/ or such.
>
> I think we should get the two build systems to produce the same installation
> layout when given equivalent options.
I'm not convinced that that's the right thing to do. Distributions have
helper infrastructure for buildsystems - why should we make it harder for them
by deviating further from the buildsystem defaults?
I have yet to hear an argument why installing libraries below
/usr/[local]/lib/{x86_64,i386,...}-linux-{gnu,musl,...}/ is the wrong thing to
do on Debian based systems (or similar, choosing lib64 over lib on RH based
systems). But at the same time I haven't heard of an argument why we should
break existing scripts building with autoconf for this. To me a different
buildsystem is a convenient point to adapt to build path from the last decade.
> Unless someone comes up with a proposal to address the above broader issues,
> also taking into account current packaging practices etc., then I think we
> should do a short-term solution to either port the subdir-appending to the
> meson scripts or remove it from the makefiles (or maybe a bit of both).
Just to be clear, with 'subdir-appending' you mean libdir defaulting to
'lib/x86_64-linux-gnu' (or similar)? Or do you mean adding 'postgresql' into
various dirs when the path doesn't already contain postgres?
I did try to mirror the 'postgresql' adding bit in the meson build.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: meson oddities
@ 2023-01-04 21:06 Robert Haas <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Robert Haas @ 2023-01-04 21:06 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Jan 4, 2023 at 2:35 PM Andres Freund <[email protected]> wrote:
> > I think we should get the two build systems to produce the same installation
> > layout when given equivalent options.
>
> I'm not convinced that that's the right thing to do. Distributions have
> helper infrastructure for buildsystems - why should we make it harder for them
> by deviating further from the buildsystem defaults?
If we don't do as Peter suggests, then any difference between the
results of one build system and the other could either be a bug or an
intentional deviation. There will be no easy way to know which it is.
And if or when people switch build systems, stuff will be randomly
different, and they won't understand why.
I hear your point too. It's unpleasant for you to spend a lot of
effort overriding meson's behavior if the result is arguably worse
than the default, and it has the effect of carrying forward in
perpetuity hacks that may not have been a good idea in the first
place, or may not be a good idea any more. Those seem like valid
concerns, too.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: meson oddities
@ 2023-01-04 22:17 Peter Eisentraut <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2023-01-04 22:17 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
On 04.01.23 20:35, Andres Freund wrote:
>> Unless someone comes up with a proposal to address the above broader issues,
>> also taking into account current packaging practices etc., then I think we
>> should do a short-term solution to either port the subdir-appending to the
>> meson scripts or remove it from the makefiles (or maybe a bit of both).
> Just to be clear, with 'subdir-appending' you mean libdir defaulting to
> 'lib/x86_64-linux-gnu' (or similar)? Or do you mean adding 'postgresql' into
> various dirs when the path doesn't already contain postgres?
>
> I did try to mirror the 'postgresql' adding bit in the meson build.
I meant the latter, which I see is already in there, but it doesn't
actually fully work. It only looks at the subdirectory (like "lib"),
not the whole path (like "/usr/local/pgsql/lib"). With the attached
patch I have it working and I get the same installation layout from both
build systems.
From 579411b5c443ce4c6f45f30627bbf891a53c8f77 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 4 Jan 2023 23:14:10 +0100
Subject: [PATCH] meson: Fix installation path computation
---
meson.build | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/meson.build b/meson.build
index 8999170b4d..634dc7d167 100644
--- a/meson.build
+++ b/meson.build
@@ -467,19 +467,19 @@ dir_prefix = get_option('prefix')
dir_bin = get_option('bindir')
dir_data = get_option('datadir')
-if not (dir_data.contains('pgsql') or dir_data.contains('postgres'))
+if not ((dir_prefix/dir_data).contains('pgsql') or (dir_prefix/dir_data).contains('postgres'))
dir_data = dir_data / pkg
endif
dir_sysconf = get_option('sysconfdir')
-if not (dir_sysconf.contains('pgsql') or dir_sysconf.contains('postgres'))
+if not ((dir_prefix/dir_sysconf).contains('pgsql') or (dir_prefix/dir_sysconf).contains('postgres'))
dir_sysconf = dir_sysconf / pkg
endif
dir_lib = get_option('libdir')
dir_lib_pkg = dir_lib
-if not (dir_lib_pkg.contains('pgsql') or dir_lib_pkg.contains('postgres'))
+if not ((dir_prefix/dir_lib_pkg).contains('pgsql') or (dir_prefix/dir_lib_pkg).contains('postgres'))
dir_lib_pkg = dir_lib_pkg / pkg
endif
@@ -489,7 +489,7 @@ dir_include = get_option('includedir')
dir_include_pkg = dir_include
dir_include_pkg_rel = ''
-if not (dir_include_pkg.contains('pgsql') or dir_include_pkg.contains('postgres'))
+if not ((dir_prefix/dir_include_pkg).contains('pgsql') or (dir_prefix/dir_include_pkg).contains('postgres'))
dir_include_pkg = dir_include_pkg / pkg
dir_include_pkg_rel = pkg
endif
--
2.39.0
Attachments:
[text/plain] 0001-meson-Fix-installation-path-computation.patch (1.6K, ../../[email protected]/2-0001-meson-Fix-installation-path-computation.patch)
download | inline diff:
From 579411b5c443ce4c6f45f30627bbf891a53c8f77 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 4 Jan 2023 23:14:10 +0100
Subject: [PATCH] meson: Fix installation path computation
---
meson.build | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/meson.build b/meson.build
index 8999170b4d..634dc7d167 100644
--- a/meson.build
+++ b/meson.build
@@ -467,19 +467,19 @@ dir_prefix = get_option('prefix')
dir_bin = get_option('bindir')
dir_data = get_option('datadir')
-if not (dir_data.contains('pgsql') or dir_data.contains('postgres'))
+if not ((dir_prefix/dir_data).contains('pgsql') or (dir_prefix/dir_data).contains('postgres'))
dir_data = dir_data / pkg
endif
dir_sysconf = get_option('sysconfdir')
-if not (dir_sysconf.contains('pgsql') or dir_sysconf.contains('postgres'))
+if not ((dir_prefix/dir_sysconf).contains('pgsql') or (dir_prefix/dir_sysconf).contains('postgres'))
dir_sysconf = dir_sysconf / pkg
endif
dir_lib = get_option('libdir')
dir_lib_pkg = dir_lib
-if not (dir_lib_pkg.contains('pgsql') or dir_lib_pkg.contains('postgres'))
+if not ((dir_prefix/dir_lib_pkg).contains('pgsql') or (dir_prefix/dir_lib_pkg).contains('postgres'))
dir_lib_pkg = dir_lib_pkg / pkg
endif
@@ -489,7 +489,7 @@ dir_include = get_option('includedir')
dir_include_pkg = dir_include
dir_include_pkg_rel = ''
-if not (dir_include_pkg.contains('pgsql') or dir_include_pkg.contains('postgres'))
+if not ((dir_prefix/dir_include_pkg).contains('pgsql') or (dir_prefix/dir_include_pkg).contains('postgres'))
dir_include_pkg = dir_include_pkg / pkg
dir_include_pkg_rel = pkg
endif
--
2.39.0
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: meson oddities
@ 2023-01-04 22:53 Andres Freund <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2023-01-04 22:53 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2023-01-04 23:17:30 +0100, Peter Eisentraut wrote:
> I meant the latter, which I see is already in there, but it doesn't actually
> fully work. It only looks at the subdirectory (like "lib"), not the whole
> path (like "/usr/local/pgsql/lib"). With the attached patch I have it
> working and I get the same installation layout from both build systems.
Oh, oops. I tested this at some point, but I guess I over-simplified it at
some point.
Then I have zero objections to this. One question below though.
> dir_data = get_option('datadir')
> -if not (dir_data.contains('pgsql') or dir_data.contains('postgres'))
> +if not ((dir_prefix/dir_data).contains('pgsql') or (dir_prefix/dir_data).contains('postgres'))
> dir_data = dir_data / pkg
> endif
Hm. Perhaps we should just test once whether prefix contains pgsql/postgres,
and then just otherwise leave the test as is? There afaict can't be a
dir_prefix/dir_* that matches postgres/pgsql that won't also match either of
the components.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: meson oddities
@ 2023-01-11 11:05 Peter Eisentraut <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Peter Eisentraut @ 2023-01-11 11:05 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
On 04.01.23 23:53, Andres Freund wrote:
>> dir_data = get_option('datadir')
>> -if not (dir_data.contains('pgsql') or dir_data.contains('postgres'))
>> +if not ((dir_prefix/dir_data).contains('pgsql') or (dir_prefix/dir_data).contains('postgres'))
>> dir_data = dir_data / pkg
>> endif
> Hm. Perhaps we should just test once whether prefix contains pgsql/postgres,
> and then just otherwise leave the test as is? There afaict can't be a
> dir_prefix/dir_* that matches postgres/pgsql that won't also match either of
> the components.
You mean something like
dir_prefix_contains_pg =
(dir_prefix.contains('pgsql') or dir_prefix.contains('postgres'))
and
if not (dir_prefix_contains_pg or
(dir_data.contains('pgsql') or dir_data.contains('postgres'))
Seems more complicated to me.
I think there is also an adjacent issue: The subdir options may be
absolute or relative. So if you specify --prefix=/usr/local and
--sysconfdir=/etc/postgresql, then
config_paths_data.set_quoted('SYSCONFDIR', dir_prefix / dir_sysconf)
would produce something like /usr/local/etc/postgresql.
I think maybe we should make all the dir_* variables absolute right at
the beginning, like
dir_lib = get_option('libdir')
if not fs.is_absolute(dir_lib)
dir_lib = dir_prefix / dir_lib
endif
And then the appending stuff could be done after that, keeping the
current code.
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-01-11 11:05 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 00:56 [PATCH 3/8] Move processing of NULLs from BRIN support functions Tomas Vondra <[email protected]>
2023-01-04 11:35 Re: meson oddities Peter Eisentraut <[email protected]>
2023-01-04 19:35 ` Re: meson oddities Andres Freund <[email protected]>
2023-01-04 21:06 ` Re: meson oddities Robert Haas <[email protected]>
2023-01-04 22:17 ` Re: meson oddities Peter Eisentraut <[email protected]>
2023-01-04 22:53 ` Re: meson oddities Andres Freund <[email protected]>
2023-01-11 11:05 ` Re: meson oddities Peter Eisentraut <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox