public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/8] Expand the external data before forming the tuple
8+ messages / 5 participants
[nested] [flat]
* [PATCH 2/8] Expand the external data before forming the tuple
@ 2021-03-04 09:25 Dilip Kumar <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Dilip Kumar @ 2021-03-04 09:25 UTC (permalink / raw)
All the callers of HeapTupleGetDatum and HeapTupleHeaderGetDatum, who might
contain the external varlena in their tuple, try to flatten the external
varlena before forming the tuple wherever it is possible.
Dilip Kumar based on idea by Robert Haas
---
src/backend/executor/execExprInterp.c | 29 +++++++++++++++++++++++++--
src/backend/utils/adt/jsonfuncs.c | 8 ++++++--
src/pl/plpgsql/src/pl_exec.c | 5 ++++-
3 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 20949a5dec..c3754acca4 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2840,12 +2840,25 @@ ExecEvalRow(ExprState *state, ExprEvalStep *op)
{
HeapTuple tuple;
+ /* Retrieve externally stored values and decompress. */
+ for (int i = 0; i < op->d.row.tupdesc->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(op->d.row.tupdesc, i);
+
+ if (op->d.row.elemnulls[i] || attr->attlen != -1 ||
+ !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.row.elemvalues[i])))
+ continue;
+
+ op->d.row.elemvalues[i] =
+ PointerGetDatum(PG_DETOAST_DATUM_PACKED(op->d.row.elemvalues[i]));
+ }
+
/* build tuple from evaluated field values */
tuple = heap_form_tuple(op->d.row.tupdesc,
op->d.row.elemvalues,
op->d.row.elemnulls);
- *op->resvalue = HeapTupleGetDatum(tuple);
+ *op->resvalue = HeapTupleGetRawDatum(tuple);
*op->resnull = false;
}
@@ -3085,12 +3098,24 @@ ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext
{
HeapTuple tuple;
+ /* Retrieve externally stored values and decompress. */
+ for (int i = 0; i < (*op->d.fieldstore.argdesc)->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(*op->d.fieldstore.argdesc, i);
+
+ if (op->d.fieldstore.nulls[i] || attr->attlen != -1 ||
+ !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.fieldstore.values[i])))
+ continue;
+ op->d.fieldstore.values[i] = PointerGetDatum(
+ PG_DETOAST_DATUM_PACKED(op->d.fieldstore.values[i]));
+ }
+
/* argdesc should already be valid from the DeForm step */
tuple = heap_form_tuple(*op->d.fieldstore.argdesc,
op->d.fieldstore.values,
op->d.fieldstore.nulls);
- *op->resvalue = HeapTupleGetDatum(tuple);
+ *op->resvalue = HeapTupleGetRawDatum(tuple);
*op->resnull = false;
}
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 511467280f..c3d464f42b 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -2968,7 +2968,7 @@ populate_composite(CompositeIOData *io,
/* populate resulting record tuple */
tuple = populate_record(io->tupdesc, &io->record_io,
defaultval, mcxt, &jso);
- result = HeapTupleHeaderGetDatum(tuple);
+ result = HeapTupleHeaderGetRawDatum(tuple);
JsObjectFree(&jso);
}
@@ -3387,6 +3387,10 @@ populate_record(TupleDesc tupdesc,
nulls[i] ? (Datum) 0 : values[i],
&field,
&nulls[i]);
+
+ if (!nulls[i] && att->attlen == -1 &&
+ VARATT_IS_EXTERNAL(DatumGetPointer(values[i])))
+ values[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(values[i]));
}
res = heap_form_tuple(tupdesc, values, nulls);
@@ -3765,7 +3769,7 @@ populate_recordset_record(PopulateRecordsetState *state, JsObject *obj)
/* if it's domain over composite, check domain constraints */
if (cache->c.typcat == TYPECAT_COMPOSITE_DOMAIN)
- domain_check(HeapTupleHeaderGetDatum(tuphead), false,
+ domain_check(HeapTupleHeaderGetRawDatum(tuphead), false,
cache->argtype,
&cache->c.io.composite.domain_info,
cache->fn_mcxt);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b4c70aaa7f..fd073767bc 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -5326,7 +5326,7 @@ exec_eval_datum(PLpgSQL_execstate *estate,
elog(ERROR, "row not compatible with its own tupdesc");
*typeid = row->rowtupdesc->tdtypeid;
*typetypmod = row->rowtupdesc->tdtypmod;
- *value = HeapTupleGetDatum(tup);
+ *value = HeapTupleGetRawDatum(tup);
*isnull = false;
MemoryContextSwitchTo(oldcontext);
break;
@@ -7300,6 +7300,9 @@ make_tuple_from_row(PLpgSQL_execstate *estate,
&dvalues[i], &nulls[i]);
if (fieldtypeid != TupleDescAttr(tupdesc, i)->atttypid)
return NULL;
+ if (!nulls[i] && TupleDescAttr(tupdesc, i)->attlen == -1 &&
+ VARATT_IS_EXTERNAL(DatumGetPointer(dvalues[i])))
+ dvalues[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(dvalues[i]));
/* XXX should we insist on typmod match, too? */
}
--
2.17.0
--cvVnyQ+4j833TQvp
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0003-Disallow-compressed-data-inside-container-types.patch"
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Hide exposed impl detail of wchar.c
@ 2023-11-16 22:54 Nathan Bossart <[email protected]>
0 siblings, 2 replies; 8+ messages in thread
From: Nathan Bossart @ 2023-11-16 22:54 UTC (permalink / raw)
To: Jubilee Young <[email protected]>; +Cc: [email protected]
On Thu, Nov 16, 2023 at 12:10:59PM -0800, Jubilee Young wrote:
> On the off-chance that everyone agrees with me without reserve, the
> attached patch moves the relevant code around (and includes the gory
> details). This seems to be unlikely to be the only mildly-exotic build
> system failure caused by such an overexposed implementation detail, so
> while I'm not married to this particular code motion, it seems best to
> improve this some way.
It looks like is_valid_ascii() was originally added to pg_wchar.h so that
it could easily be used elsewhere [0] [1], but that doesn't seem to have
happened yet.
Would moving this definition to a separate header file be a viable option?
That'd still break any existing projects that are using it, but at least
there'd be an easy fix. I'm not sure there _are_ any other projects using
it, anyway. However, both of these proposals feel like they might be
slightly beyond what we'd ordinarily consider back-patching.
That being said, it's not unheard of for Postgres to make adjustments for
third-party code (search for "pljava" in commits 62aba76 and f4aa3a1). I
read the description of the pgrx problem [2], and I'm still trying to
understand the scope of the issue. I don't think it's reasonable to expect
someone building an extension to always use the exact same compiler that
was used by the packager, but I also don't fully understand why different
compilations of an inline function would cause problems.
[0] https://postgr.es/m/CAFBsxsHG%3Dg6W8Mie%2B_NO8dV6O0pO2stxrnS%3Dme5ZmGqk--fd5g%40mail.gmail.com
[1] https://postgr.es/m/CAFBsxsH1Yutrmu%2B6LLHKK8iXY%2BvG--Do6zN%2B2900spHXQNNQKQ%40mail.gmail.com
[2] https://github.com/pgcentralfoundation/pgrx/issues/1298
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Hide exposed impl detail of wchar.c
@ 2023-11-16 23:06 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: Tom Lane @ 2023-11-16 23:06 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Jubilee Young <[email protected]>; [email protected]; John Naylor <[email protected]>
Nathan Bossart <[email protected]> writes:
> It looks like is_valid_ascii() was originally added to pg_wchar.h so that
> it could easily be used elsewhere [0] [1], but that doesn't seem to have
> happened yet.
It seems to be new as of v15, so there wouldn't have been a lot of time
for external code to adopt it. As far as I can tell from Debian Code
Search, nobody has yet.
> Would moving this definition to a separate header file be a viable option?
> That'd still break any existing projects that are using it, but at least
> there'd be an easy fix.
That would provide a little bit of cover, at least, compared to just
hiding it in the .c file.
I'm generally sympathetic to the idea that simd.h was a rather large
dependency to add to something as widely used as pg_wchar.h. So I'd
favor getting it out of there just on compilation-time grounds,
independently of whether it's causing active problems. That argument
wouldn't justify a back-patch, but "it's causing problems" might.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Hide exposed impl detail of wchar.c
@ 2023-11-17 04:38 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Nathan Bossart @ 2023-11-17 04:38 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Jubilee Young <[email protected]>; [email protected]; John Naylor <[email protected]>
On Thu, Nov 16, 2023 at 06:06:30PM -0500, Tom Lane wrote:
> I'm generally sympathetic to the idea that simd.h was a rather large
> dependency to add to something as widely used as pg_wchar.h. So I'd
> favor getting it out of there just on compilation-time grounds,
> independently of whether it's causing active problems. That argument
> wouldn't justify a back-patch, but "it's causing problems" might.
Given the lack of evidence of anyone else using is_valid_ascii(), I'm
leaning towards back-patching being the better option in this case. I
don't know if it'll be feasible to keep simd.h out of all headers that
third-party code might want to use forever, but that's not an argument
against doing this right now for pgrx.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Hide exposed impl detail of wchar.c
@ 2023-11-17 10:26 John Naylor <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: John Naylor @ 2023-11-17 10:26 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Jubilee Young <[email protected]>; [email protected]
On Fri, Nov 17, 2023 at 5:54 AM Nathan Bossart <[email protected]> wrote:
>
> It looks like is_valid_ascii() was originally added to pg_wchar.h so that
> it could easily be used elsewhere [0] [1], but that doesn't seem to have
> happened yet.
>
> Would moving this definition to a separate header file be a viable option?
Seems fine to me. (I believe the original motivation for making it an
inline function was for in pg_mbstrlen_with_len(), but trying that
hasn't been a priority.)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Hide exposed impl detail of wchar.c
@ 2023-11-20 18:50 Jubilee Young <[email protected]>
parent: John Naylor <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Jubilee Young @ 2023-11-20 18:50 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: Nathan Bossart <[email protected]>; [email protected]
On Fri, Nov 17, 2023 at 2:26 AM John Naylor <[email protected]> wrote:
>
> On Fri, Nov 17, 2023 at 5:54 AM Nathan Bossart <[email protected]> wrote:
> >
> > It looks like is_valid_ascii() was originally added to pg_wchar.h so that
> > it could easily be used elsewhere [0] [1], but that doesn't seem to have
> > happened yet.
> >
> > Would moving this definition to a separate header file be a viable option?
>
> Seems fine to me. (I believe the original motivation for making it an
> inline function was for in pg_mbstrlen_with_len(), but trying that
> hasn't been a priority.)
In that case, I took a look across the codebase and saw a
utils/ascii.h that doesn't
seem to have gotten much love, but I suppose one could argue that it's intended
to be a backend-only header file?
As the codebase is growing some enhanced UTF-8 support, you'll want somewhere
that contains the optimized US-ASCII routines, because, as US-ASCII is
a subset of
UTF-8, and often faster to handle, it's typical for such codepaths to look like
```c
while (i < len && no_multibyte_chars) {
i = i + ascii_op_version(i, buffer, &no_multibyte_chars);
}
while (i < len) {
i = i + utf8_op_version(i, buffer);
}
```
So it should probably end up living somewhere near the UTF-8 support, and
the easiest way to make it not go into something pgrx currently
includes would be
to make it a new header file, though there's a fair amount of API we
don't touch.
From the pgrx / Rust perspective, Postgres function calls are passed
via callback
to a "guard function" that guarantees that longjmp and setjmp don't
cause trouble
(and makes sure we participate in that). So we only want to call
Postgres functions
if we "can't replace" them, as the overhead is quite a lot. That means
UTF-8-per-se
functions aren't very interesting to us as the Rust language already
supports it, but
we do benefit from access to transcoding to/from UTF-8.
—Jubilee
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Hide exposed impl detail of wchar.c
@ 2023-11-20 22:52 Nathan Bossart <[email protected]>
parent: Jubilee Young <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2023-11-20 22:52 UTC (permalink / raw)
To: Jubilee Young <[email protected]>; +Cc: John Naylor <[email protected]>; [email protected]
On Mon, Nov 20, 2023 at 10:50:36AM -0800, Jubilee Young wrote:
> In that case, I took a look across the codebase and saw a
> utils/ascii.h that doesn't
> seem to have gotten much love, but I suppose one could argue that it's intended
> to be a backend-only header file?
That might work. It's not #included in very many files, so adding
port/simd.h shouldn't be too bad. And ascii.h is also pretty inexpensive,
so including it in wchar.c seems permissible, too. I'm not certain this
doesn't cause problems with libpgcommon, but I don't see why it would,
either.
> So it should probably end up living somewhere near the UTF-8 support, and
> the easiest way to make it not go into something pgrx currently
> includes would be
> to make it a new header file, though there's a fair amount of API we
> don't touch.
Does pgrx use ascii.h at all?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] move_is_valid_ascii_v2.patch (4.8K, ../../20231120225222.GA3474027@nathanxps13/2-move_is_valid_ascii_v2.patch)
download | inline diff:
diff --git a/src/common/wchar.c b/src/common/wchar.c
index fb9d9f5c85..fbac11deb4 100644
--- a/src/common/wchar.c
+++ b/src/common/wchar.c
@@ -13,6 +13,7 @@
#include "c.h"
#include "mb/pg_wchar.h"
+#include "utils/ascii.h"
/*
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index 29cd5732f1..80676d9e02 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -22,8 +22,6 @@
#ifndef PG_WCHAR_H
#define PG_WCHAR_H
-#include "port/simd.h"
-
/*
* The pg_wchar type
*/
@@ -722,71 +720,4 @@ extern int mic2latin_with_table(const unsigned char *mic, unsigned char *p,
extern WCHAR *pgwin32_message_to_UTF16(const char *str, int len, int *utf16len);
#endif
-
-/*
- * Verify a chunk of bytes for valid ASCII.
- *
- * Returns false if the input contains any zero bytes or bytes with the
- * high-bit set. Input len must be a multiple of the chunk size (8 or 16).
- */
-static inline bool
-is_valid_ascii(const unsigned char *s, int len)
-{
- const unsigned char *const s_end = s + len;
- Vector8 chunk;
- Vector8 highbit_cum = vector8_broadcast(0);
-#ifdef USE_NO_SIMD
- Vector8 zero_cum = vector8_broadcast(0x80);
-#endif
-
- Assert(len % sizeof(chunk) == 0);
-
- while (s < s_end)
- {
- vector8_load(&chunk, s);
-
- /* Capture any zero bytes in this chunk. */
-#ifdef USE_NO_SIMD
-
- /*
- * First, add 0x7f to each byte. This sets the high bit in each byte,
- * unless it was a zero. If any resulting high bits are zero, the
- * corresponding high bits in the zero accumulator will be cleared.
- *
- * If none of the bytes in the chunk had the high bit set, the max
- * value each byte can have after the addition is 0x7f + 0x7f = 0xfe,
- * and we don't need to worry about carrying over to the next byte. If
- * any input bytes did have the high bit set, it doesn't matter
- * because we check for those separately.
- */
- zero_cum &= (chunk + vector8_broadcast(0x7F));
-#else
-
- /*
- * Set all bits in each lane of the highbit accumulator where input
- * bytes are zero.
- */
- highbit_cum = vector8_or(highbit_cum,
- vector8_eq(chunk, vector8_broadcast(0)));
-#endif
-
- /* Capture all set bits in this chunk. */
- highbit_cum = vector8_or(highbit_cum, chunk);
-
- s += sizeof(chunk);
- }
-
- /* Check if any high bits in the high bit accumulator got set. */
- if (vector8_is_highbit_set(highbit_cum))
- return false;
-
-#ifdef USE_NO_SIMD
- /* Check if any high bits in the zero accumulator got cleared. */
- if (zero_cum != vector8_broadcast(0x80))
- return false;
-#endif
-
- return true;
-}
-
#endif /* PG_WCHAR_H */
diff --git a/src/include/utils/ascii.h b/src/include/utils/ascii.h
index 630acd9bfd..7df024dad3 100644
--- a/src/include/utils/ascii.h
+++ b/src/include/utils/ascii.h
@@ -11,6 +11,74 @@
#ifndef _ASCII_H_
#define _ASCII_H_
+#include "port/simd.h"
+
extern void ascii_safe_strlcpy(char *dest, const char *src, size_t destsiz);
+/*
+ * Verify a chunk of bytes for valid ASCII.
+ *
+ * Returns false if the input contains any zero bytes or bytes with the
+ * high-bit set. Input len must be a multiple of the chunk size (8 or 16).
+ */
+static inline bool
+is_valid_ascii(const unsigned char *s, int len)
+{
+ const unsigned char *const s_end = s + len;
+ Vector8 chunk;
+ Vector8 highbit_cum = vector8_broadcast(0);
+#ifdef USE_NO_SIMD
+ Vector8 zero_cum = vector8_broadcast(0x80);
+#endif
+
+ Assert(len % sizeof(chunk) == 0);
+
+ while (s < s_end)
+ {
+ vector8_load(&chunk, s);
+
+ /* Capture any zero bytes in this chunk. */
+#ifdef USE_NO_SIMD
+
+ /*
+ * First, add 0x7f to each byte. This sets the high bit in each byte,
+ * unless it was a zero. If any resulting high bits are zero, the
+ * corresponding high bits in the zero accumulator will be cleared.
+ *
+ * If none of the bytes in the chunk had the high bit set, the max
+ * value each byte can have after the addition is 0x7f + 0x7f = 0xfe,
+ * and we don't need to worry about carrying over to the next byte. If
+ * any input bytes did have the high bit set, it doesn't matter
+ * because we check for those separately.
+ */
+ zero_cum &= (chunk + vector8_broadcast(0x7F));
+#else
+
+ /*
+ * Set all bits in each lane of the highbit accumulator where input
+ * bytes are zero.
+ */
+ highbit_cum = vector8_or(highbit_cum,
+ vector8_eq(chunk, vector8_broadcast(0)));
+#endif
+
+ /* Capture all set bits in this chunk. */
+ highbit_cum = vector8_or(highbit_cum, chunk);
+
+ s += sizeof(chunk);
+ }
+
+ /* Check if any high bits in the high bit accumulator got set. */
+ if (vector8_is_highbit_set(highbit_cum))
+ return false;
+
+#ifdef USE_NO_SIMD
+ /* Check if any high bits in the zero accumulator got cleared. */
+ if (zero_cum != vector8_broadcast(0x80))
+ return false;
+#endif
+
+ return true;
+}
+
#endif /* _ASCII_H_ */
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Hide exposed impl detail of wchar.c
@ 2023-11-21 04:39 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Nathan Bossart @ 2023-11-21 04:39 UTC (permalink / raw)
To: Jubilee Young <[email protected]>; +Cc: pgsql-hackers; [email protected]; [email protected]; [email protected]
On Mon, Nov 20, 2023 at 05:14:17PM -0800, Jubilee Young wrote:
> On Mon, Nov 20, 2023 at 2:52 PM Nathan Bossart <[email protected]> wrote:
>> Does pgrx use ascii.h at all?
>
> We don't use utils/ascii.h, no.
Alright. The next minor release isn't until February, so I'll let this one
sit a little while longer in case anyone objects to back-patching something
like this [0].
[0] https://postgr.es/m/attachment/152305/move_is_valid_ascii_v2.patch
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-11-21 04:39 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 09:25 [PATCH 2/8] Expand the external data before forming the tuple Dilip Kumar <[email protected]>
2023-11-16 22:54 Re: Hide exposed impl detail of wchar.c Nathan Bossart <[email protected]>
2023-11-16 23:06 ` Re: Hide exposed impl detail of wchar.c Tom Lane <[email protected]>
2023-11-17 04:38 ` Re: Hide exposed impl detail of wchar.c Nathan Bossart <[email protected]>
2023-11-17 10:26 ` Re: Hide exposed impl detail of wchar.c John Naylor <[email protected]>
2023-11-20 18:50 ` Re: Hide exposed impl detail of wchar.c Jubilee Young <[email protected]>
2023-11-20 22:52 ` Re: Hide exposed impl detail of wchar.c Nathan Bossart <[email protected]>
2023-11-21 04:39 ` Re: Hide exposed impl detail of wchar.c Nathan Bossart <[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