Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)
Hi,
At function has_matching_range, if variable ranges->nranges == 0,
we exit quickly with a result equal to false.
This means that nranges can be zero.
It occurs then that it is possible then to occur an array out of bonds, in
the initialization of the variable maxvalue.
So if nranges is equal to zero, there is no need to initialize minvalue and
maxvalue.
The patch tries to fix it, avoiding possible errors by using maxvalue.
regards,
Ranier Vilela
Attachments:
0001-fix-bogus-possible-array-out-bonds.patchapplication/octet-stream; name=0001-fix-bogus-possible-array-out-bonds.patchDownload
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index a581659fe2..538cedc784 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -923,8 +923,8 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
{
Datum compar;
- Datum minvalue = ranges->values[0];
- Datum maxvalue = ranges->values[2 * ranges->nranges - 1];
+ Datum minvalue;
+ Datum maxvalue;
FmgrInfo *cmpLessFn;
FmgrInfo *cmpGreaterFn;
@@ -936,6 +936,9 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
if (ranges->nranges == 0)
return false;
+ minvalue = ranges->values[0];
+ maxvalue = ranges->values[2 * ranges->nranges - 1];
+
/*
* Otherwise, need to compare the new value with boundaries of all the
* ranges. First check if it's less than the absolute minimum, which is
At Fri, 26 Aug 2022 10:28:50 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
At function has_matching_range, if variable ranges->nranges == 0,
we exit quickly with a result equal to false.This means that nranges can be zero.
It occurs then that it is possible then to occur an array out of bonds, in
the initialization of the variable maxvalue.
So if nranges is equal to zero, there is no need to initialize minvalue and
maxvalue.The patch tries to fix it, avoiding possible errors by using maxvalue.
However it seems that nranges will never be zero, still the fix looks
good to me since it is generally allowed to be zero. I don't find a
similar mistake related to Range.nranges.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Em dom., 28 de ago. de 2022 às 22:06, Kyotaro Horiguchi <
horikyota.ntt@gmail.com> escreveu:
At Fri, 26 Aug 2022 10:28:50 -0300, Ranier Vilela <ranier.vf@gmail.com>
wrote inAt function has_matching_range, if variable ranges->nranges == 0,
we exit quickly with a result equal to false.This means that nranges can be zero.
It occurs then that it is possible then to occur an array out of bonds,in
the initialization of the variable maxvalue.
So if nranges is equal to zero, there is no need to initialize minvalueand
maxvalue.
The patch tries to fix it, avoiding possible errors by using maxvalue.
However it seems that nranges will never be zero, still the fix looks
good to me since it is generally allowed to be zero. I don't find a
similar mistake related to Range.nranges.
Thanks Kyotaro for taking a look at this.
regards,
Ranier Vilela
On Sat, 27 Aug 2022 at 01:29, Ranier Vilela <ranier.vf@gmail.com> wrote:
At function has_matching_range, if variable ranges->nranges == 0,
we exit quickly with a result equal to false.This means that nranges can be zero.
It occurs then that it is possible then to occur an array out of bonds, in the initialization of the variable maxvalue.
So if nranges is equal to zero, there is no need to initialize minvalue and maxvalue.
I think there's more strange coding in the same file that might need
addressed, for example, AssertCheckRanges() does:
if (ranges->nranges == 0)
break;
from within the first for() loop. Why can't that check be outside of
the loop. Nothing seems to make any changes to that field from within
the loop.
Also, in the final loop of the same function there's:
if (ranges->nsorted == 0)
break;
It's not very obvious to me why we don't only run that loop when
ranges->nsorted > 0. Also, isn't it an array overrun to access:
Datum value = ranges->values[2 * ranges->nranges + i];
If there's only 1 range stored in the array, then there should be 2
elements, but that code will try to access the 3rd element with
ranges->values[2].
This is not so critical, but I'll note it down anyway. The following
looks a bit suboptimal in brin_minmax_multi_summary_out():
StringInfoData str;
initStringInfo(&str);
a = FunctionCall1(&fmgrinfo, ranges_deserialized->values[idx++]);
appendStringInfoString(&str, DatumGetCString(a));
b = cstring_to_text(str.data);
Why do we need a StringInfoData there? Why not just do:
b = cstring_to_text(DatumGetCString(a)); ?
That requires less memcpy()s and pallocs().
I've included Tomas just in case I've misunderstood the nrange stuff.
David
Em qui., 1 de set. de 2022 às 21:27, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Sat, 27 Aug 2022 at 01:29, Ranier Vilela <ranier.vf@gmail.com> wrote:
At function has_matching_range, if variable ranges->nranges == 0,
we exit quickly with a result equal to false.This means that nranges can be zero.
It occurs then that it is possible then to occur an array out of bonds,in the initialization of the variable maxvalue.
So if nranges is equal to zero, there is no need to initialize minvalue
and maxvalue.
I think there's more strange coding in the same file that might need
addressed, for example, AssertCheckRanges() does:if (ranges->nranges == 0)
break;from within the first for() loop. Why can't that check be outside of
the loop. Nothing seems to make any changes to that field from within
the loop.Also, in the final loop of the same function there's:
if (ranges->nsorted == 0)
break;It's not very obvious to me why we don't only run that loop when
ranges->nsorted > 0. Also, isn't it an array overrun to access:Datum value = ranges->values[2 * ranges->nranges + i];
If there's only 1 range stored in the array, then there should be 2
elements, but that code will try to access the 3rd element with
ranges->values[2].
Yeah, it seems to me that both nranges and nsorted are invariant there,
so we can safely avoid loops.
This is not so critical, but I'll note it down anyway. The following
looks a bit suboptimal in brin_minmax_multi_summary_out():StringInfoData str;
initStringInfo(&str);
a = FunctionCall1(&fmgrinfo, ranges_deserialized->values[idx++]);
appendStringInfoString(&str, DatumGetCString(a));
b = cstring_to_text(str.data);
Why do we need a StringInfoData there? Why not just do:
b = cstring_to_text(DatumGetCString(a)); ?
That requires less memcpy()s and pallocs().
I agree that StringInfoData is not needed there.
Is it strange to convert char * to only store a temporary str.data.
Why not?
astate_values = accumArrayResult(astate_values,
PointerGetDatum(a),
false,
TEXTOID,
CurrentMemoryContext);
Is it possible to avoid cstring_to_text conversion?
regards,
Ranier Vilela
On Fri, 2 Sept 2022 at 12:55, Ranier Vilela <ranier.vf@gmail.com> wrote:
Why not?
astate_values = accumArrayResult(astate_values,
PointerGetDatum(a),
false,
TEXTOID,
CurrentMemoryContext);Is it possible to avoid cstring_to_text conversion?
Note the element_type is being passed to accumArrayResult() as
TEXTOID, so we should be passing a text type, not a cstring type. The
conversion to text is required.
David
On Mon, Aug 29, 2022 at 10:06:55AM +0900, Kyotaro Horiguchi wrote:
At Fri, 26 Aug 2022 10:28:50 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
At function has_matching_range, if variable ranges->nranges == 0,
we exit quickly with a result equal to false.This means that nranges can be zero.
It occurs then that it is possible then to occur an array out of bonds, in
the initialization of the variable maxvalue.
So if nranges is equal to zero, there is no need to initialize minvalue and
maxvalue.The patch tries to fix it, avoiding possible errors by using maxvalue.
However it seems that nranges will never be zero, still the fix looks
good to me since it is generally allowed to be zero. I don't find a
similar mistake related to Range.nranges.
Actually, the nranges==0 branch is hit during regression tests:
https://coverage.postgresql.org/src/backend/access/brin/brin_minmax_multi.c.gcov.html
I'm not sure, but I *suspect* that compilers usually check
ranges->nranges==0
before reading ranges->values[2 * ranges->nranges - 1];
Especially since it's a static function.
Even if they didn't (say, under -O0), values[-1] would probably point to
a palloc header, which would be enough to "not crash" before returning
one line later.
But +1 to fix this and other issues even if they would never crash.
--
Justin
Em sex., 2 de set. de 2022 às 09:01, Justin Pryzby <pryzby@telsasoft.com>
escreveu:
On Mon, Aug 29, 2022 at 10:06:55AM +0900, Kyotaro Horiguchi wrote:
At Fri, 26 Aug 2022 10:28:50 -0300, Ranier Vilela <ranier.vf@gmail.com>
wrote in
At function has_matching_range, if variable ranges->nranges == 0,
we exit quickly with a result equal to false.This means that nranges can be zero.
It occurs then that it is possible then to occur an array out ofbonds, in
the initialization of the variable maxvalue.
So if nranges is equal to zero, there is no need to initializeminvalue and
maxvalue.
The patch tries to fix it, avoiding possible errors by using maxvalue.
However it seems that nranges will never be zero, still the fix looks
good to me since it is generally allowed to be zero. I don't find a
similar mistake related to Range.nranges.Actually, the nranges==0 branch is hit during regression tests:
https://coverage.postgresql.org/src/backend/access/brin/brin_minmax_multi.c.gcov.html
I'm not sure, but I *suspect* that compilers usually check
ranges->nranges==0
before reading ranges->values[2 * ranges->nranges - 1];Especially since it's a static function.
Even if they didn't (say, under -O0), values[-1] would probably point to
a palloc header, which would be enough to "not crash" before returning
one line later.But +1 to fix this and other issues even if they would never crash.
Thanks Justin.
Based on comments by David, I made a new patch.
To simplify I've included the 0001 in the 0002 patch.
Summary:
1. Once that ranges->nranges is invariant, avoid the loop if
ranges->nranges <= 0.
This matches the current behavior.
2. Once that ranges->nsorted is invariant, avoid the loop if
ranges->nsorted <= 0.
This matches the current behavior.
3. Remove the invariant cxt from ranges->nsorted loop.
4. Avoid possible overflows when using int to store length strings.
5. Avoid possible out-of-bounds when ranges->nranges == 0.
6. Avoid overhead when using unnecessary StringInfoData to convert Datum a
to Text b.
Attached is 0002.
regards,
Ranier Vilela
Attachments:
0002-avoid-small-issues-brin_minmax_multi.patchapplication/octet-stream; name=0002-avoid-small-issues-brin_minmax_multi.patchDownload
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index a581659fe2..e8f76f1844 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -318,102 +318,103 @@ AssertCheckRanges(Ranges *ranges, FmgrInfo *cmpFn, Oid colloid)
* Check that none of the values are not covered by ranges (both sorted
* and unsorted)
*/
- for (i = 0; i < ranges->nvalues; i++)
+ if (ranges->nranges > 0)
{
- Datum compar;
- int start,
- end;
- Datum minvalue,
- maxvalue;
-
- Datum value = ranges->values[2 * ranges->nranges + i];
-
- if (ranges->nranges == 0)
- break;
-
- minvalue = ranges->values[0];
- maxvalue = ranges->values[2 * ranges->nranges - 1];
-
- /*
- * Is the value smaller than the minval? If yes, we'll recurse to the
- * left side of range array.
- */
- compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
-
- /* smaller than the smallest value in the first range */
- if (DatumGetBool(compar))
- continue;
-
- /*
- * Is the value greater than the maxval? If yes, we'll recurse to the
- * right side of range array.
- */
- compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
-
- /* larger than the largest value in the last range */
- if (DatumGetBool(compar))
- continue;
-
- start = 0; /* first range */
- end = ranges->nranges - 1; /* last range */
- while (true)
+ for (i = 0; i < ranges->nvalues; i++)
{
- int midpoint = (start + end) / 2;
+ Datum compar;
+ int start,
+ end;
+ Datum minvalue,
+ maxvalue;
- /* this means we ran out of ranges in the last step */
- if (start > end)
- break;
+ Datum value = ranges->values[2 * ranges->nranges + i];
- /* copy the min/max values from the ranges */
- minvalue = ranges->values[2 * midpoint];
- maxvalue = ranges->values[2 * midpoint + 1];
+ minvalue = ranges->values[0];
+ maxvalue = ranges->values[2 * ranges->nranges - 1];
/*
- * Is the value smaller than the minval? If yes, we'll recurse to
- * the left side of range array.
+ * Is the value smaller than the minval? If yes, we'll recurse to the
+ * left side of range array.
*/
compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
- /* smaller than the smallest value in this range */
+ /* smaller than the smallest value in the first range */
if (DatumGetBool(compar))
- {
- end = (midpoint - 1);
continue;
- }
/*
- * Is the value greater than the minval? If yes, we'll recurse to
- * the right side of range array.
+ * Is the value greater than the maxval? If yes, we'll recurse to the
+ * right side of range array.
*/
compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
- /* larger than the largest value in this range */
+ /* larger than the largest value in the last range */
if (DatumGetBool(compar))
- {
- start = (midpoint + 1);
continue;
- }
- /* hey, we found a matching range */
- Assert(false);
+ start = 0; /* first range */
+ end = ranges->nranges - 1; /* last range */
+ while (true)
+ {
+ int midpoint = (start + end) / 2;
+
+ /* this means we ran out of ranges in the last step */
+ if (start > end)
+ break;
+
+ /* copy the min/max values from the ranges */
+ minvalue = ranges->values[2 * midpoint];
+ maxvalue = ranges->values[2 * midpoint + 1];
+
+ /*
+ * Is the value smaller than the minval? If yes, we'll recurse to
+ * the left side of range array.
+ */
+ compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
+
+ /* smaller than the smallest value in this range */
+ if (DatumGetBool(compar))
+ {
+ end = (midpoint - 1);
+ continue;
+ }
+
+ /*
+ * Is the value greater than the minval? If yes, we'll recurse to
+ * the right side of range array.
+ */
+ compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
+
+ /* larger than the largest value in this range */
+ if (DatumGetBool(compar))
+ {
+ start = (midpoint + 1);
+ continue;
+ }
+
+ /* hey, we found a matching range */
+ Assert(false);
+ }
}
}
/* and values in the unsorted part must not be in sorted part */
- for (i = ranges->nsorted; i < ranges->nvalues; i++)
+ if (ranges->nsorted > 0)
{
compare_context cxt;
- Datum value = ranges->values[2 * ranges->nranges + i];
-
- if (ranges->nsorted == 0)
- break;
cxt.colloid = ranges->colloid;
cxt.cmpFn = ranges->cmp;
- Assert(bsearch_arg(&value, &ranges->values[2 * ranges->nranges],
- ranges->nsorted, sizeof(Datum),
- compare_values, (void *) &cxt) == NULL);
+ for (i = ranges->nsorted; i < ranges->nvalues; i++)
+ {
+ Datum value = ranges->values[2 * ranges->nranges + i];
+
+ Assert(bsearch_arg(&value, &ranges->values[2 * ranges->nranges],
+ ranges->nsorted, sizeof(Datum),
+ compare_values, (void *) &cxt) == NULL);
+ }
}
#endif
}
@@ -687,14 +688,14 @@ brin_range_serialize(Ranges *range)
}
else if (typlen == -1) /* varlena */
{
- int tmp = VARSIZE_ANY(DatumGetPointer(range->values[i]));
+ Size tmp = VARSIZE_ANY(DatumGetPointer(range->values[i]));
memcpy(ptr, DatumGetPointer(range->values[i]), tmp);
ptr += tmp;
}
else if (typlen == -2) /* cstring */
{
- int tmp = strlen(DatumGetCString(range->values[i])) + 1;
+ Size tmp = strlen(DatumGetCString(range->values[i])) + 1;
memcpy(ptr, DatumGetCString(range->values[i]), tmp);
ptr += tmp;
@@ -923,8 +924,8 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
{
Datum compar;
- Datum minvalue = ranges->values[0];
- Datum maxvalue = ranges->values[2 * ranges->nranges - 1];
+ Datum minvalue;
+ Datum maxvalue;
FmgrInfo *cmpLessFn;
FmgrInfo *cmpGreaterFn;
@@ -936,6 +937,9 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
if (ranges->nranges == 0)
return false;
+ minvalue = ranges->values[0];
+ maxvalue = ranges->values[2 * ranges->nranges - 1];
+
/*
* Otherwise, need to compare the new value with boundaries of all the
* ranges. First check if it's less than the absolute minimum, which is
@@ -3095,15 +3099,9 @@ brin_minmax_multi_summary_out(PG_FUNCTION_ARGS)
{
Datum a;
text *b;
- StringInfoData str;
-
- initStringInfo(&str);
a = FunctionCall1(&fmgrinfo, ranges_deserialized->values[idx++]);
-
- appendStringInfoString(&str, DatumGetCString(a));
-
- b = cstring_to_text(str.data);
+ b = cstring_to_text(DatumGetCString(a));
astate_values = accumArrayResult(astate_values,
PointerGetDatum(b),
On Sat, 3 Sept 2022 at 00:37, Ranier Vilela <ranier.vf@gmail.com> wrote:
But +1 to fix this and other issues even if they would never crash.
Yeah, I don't think any of this coding would lead to a crash, but it's
pretty weird coding and we should fix it.
1. Once that ranges->nranges is invariant, avoid the loop if ranges->nranges <= 0.
This matches the current behavior.2. Once that ranges->nsorted is invariant, avoid the loop if ranges->nsorted <= 0.
This matches the current behavior.3. Remove the invariant cxt from ranges->nsorted loop.
4. Avoid possible overflows when using int to store length strings.
5. Avoid possible out-of-bounds when ranges->nranges == 0.
6. Avoid overhead when using unnecessary StringInfoData to convert Datum a to Text b.
I've ripped out #4 and #6 for now. I think we should do #6 in master
only, probably as part of a wider cleanup of StringInfo misusages.
I also spent some time trying to ensure I understand this code
correctly. I was unable to work out what the nsorted field was from
just the comments. I needed to look at the code to figure it out, so I
think the comments for that field need to be improved. A few of the
others were not that clear either. I hope I've improved them in the
attached.
I was also a bit confused at various other comments. e.g:
/*
* Is the value greater than the maxval? If yes, we'll recurse to the
* right side of range array.
*/
I don't see any sort of recursion going on here. All I see are
skipping of values that are out of bounds of the lower bound of the
lowest range, and above the upper bound of the highest range.
I propose to backpatch the attached into v14 tomorrow morning (about
12 hours from now).
The only other weird coding I found was in brin_range_deserialize:
for (i = 0; (i < nvalues) && (!typbyval); i++)
I imagine most compilers would optimize that so that the typbyval
check is done before the first loop and not done on every loop, but I
don't think that coding practice helps the human readers out much. I
left that one alone, for now.
David
Attachments:
v3_avoid_referencing_out_of_bounds_array_elements.patchtext/plain; charset=US-ASCII; name=v3_avoid_referencing_out_of_bounds_array_elements.patchDownload
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index a581659fe2..99ef97206a 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -149,13 +149,16 @@ typedef struct MinMaxMultiOptions
* single-point ranges. That is, we have (2*nranges + nvalues) boundary
* values in the array.
*
- * +---------------------------------+-------------------------------+
- * | ranges (sorted pairs of values) | sorted values (single points) |
- * +---------------------------------+-------------------------------+
+ * +-------------------------+----------------------------------+
+ * | ranges (2 * nranges of) | single point values (nvalues of) |
+ * +-------------------------+----------------------------------+
*
* This allows us to quickly add new values, and store outliers without
* making the other ranges very wide.
*
+ * 'nsorted' denotes how many of 'nvalues' in the values[] array are sorted.
+ * When nsorted == nvalues, all single point values are sorted.
+ *
* We never store more than maxvalues values (as set by values_per_range
* reloption). If needed we merge some of the ranges.
*
@@ -173,10 +176,10 @@ typedef struct Ranges
FmgrInfo *cmp;
/* (2*nranges + nvalues) <= maxvalues */
- int nranges; /* number of ranges in the array (stored) */
- int nsorted; /* number of sorted values (ranges + points) */
- int nvalues; /* number of values in the data array (all) */
- int maxvalues; /* maximum number of values (reloption) */
+ int nranges; /* number of ranges in the values[] array */
+ int nsorted; /* number of nvalues which are sorted */
+ int nvalues; /* number of point values in values[] */
+ int maxvalues; /* number of elements in the values[] array */
/*
* We simply add the values into a large buffer, without any expensive
@@ -318,102 +321,99 @@ AssertCheckRanges(Ranges *ranges, FmgrInfo *cmpFn, Oid colloid)
* Check that none of the values are not covered by ranges (both sorted
* and unsorted)
*/
- for (i = 0; i < ranges->nvalues; i++)
+ if (ranges->nranges > 0)
{
- Datum compar;
- int start,
- end;
- Datum minvalue,
- maxvalue;
-
- Datum value = ranges->values[2 * ranges->nranges + i];
-
- if (ranges->nranges == 0)
- break;
-
- minvalue = ranges->values[0];
- maxvalue = ranges->values[2 * ranges->nranges - 1];
-
- /*
- * Is the value smaller than the minval? If yes, we'll recurse to the
- * left side of range array.
- */
- compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
-
- /* smaller than the smallest value in the first range */
- if (DatumGetBool(compar))
- continue;
-
- /*
- * Is the value greater than the maxval? If yes, we'll recurse to the
- * right side of range array.
- */
- compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
-
- /* larger than the largest value in the last range */
- if (DatumGetBool(compar))
- continue;
-
- start = 0; /* first range */
- end = ranges->nranges - 1; /* last range */
- while (true)
+ for (i = 0; i < ranges->nvalues; i++)
{
- int midpoint = (start + end) / 2;
-
- /* this means we ran out of ranges in the last step */
- if (start > end)
- break;
+ Datum compar;
+ int start,
+ end;
+ Datum minvalue = ranges->values[0];
+ Datum maxvalue = ranges->values[2 * ranges->nranges - 1];
+ Datum value = ranges->values[2 * ranges->nranges + i];
- /* copy the min/max values from the ranges */
- minvalue = ranges->values[2 * midpoint];
- maxvalue = ranges->values[2 * midpoint + 1];
+ compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
/*
- * Is the value smaller than the minval? If yes, we'll recurse to
- * the left side of range array.
+ * If the value is smaller than the lower bound in the first range
+ * then it cannot possibly be in any of the ranges.
*/
- compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
-
- /* smaller than the smallest value in this range */
if (DatumGetBool(compar))
- {
- end = (midpoint - 1);
continue;
- }
- /*
- * Is the value greater than the minval? If yes, we'll recurse to
- * the right side of range array.
- */
compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
- /* larger than the largest value in this range */
+ /*
+ * Likewise, if the value is larger than the upper bound of the
+ * final range, then it cannot possibly be inside any of the
+ * ranges.
+ */
if (DatumGetBool(compar))
- {
- start = (midpoint + 1);
continue;
- }
- /* hey, we found a matching range */
- Assert(false);
+ /* bsearch the ranges to see if 'value' fits within any of them */
+ start = 0; /* first range */
+ end = ranges->nranges - 1; /* last range */
+ while (true)
+ {
+ int midpoint = (start + end) / 2;
+
+ /* this means we ran out of ranges in the last step */
+ if (start > end)
+ break;
+
+ /* copy the min/max values from the ranges */
+ minvalue = ranges->values[2 * midpoint];
+ maxvalue = ranges->values[2 * midpoint + 1];
+
+ /*
+ * Is the value smaller than the minval? If yes, we'll recurse
+ * to the left side of range array.
+ */
+ compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
+
+ /* smaller than the smallest value in this range */
+ if (DatumGetBool(compar))
+ {
+ end = (midpoint - 1);
+ continue;
+ }
+
+ /*
+ * Is the value greater than the minval? If yes, we'll recurse
+ * to the right side of range array.
+ */
+ compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
+
+ /* larger than the largest value in this range */
+ if (DatumGetBool(compar))
+ {
+ start = (midpoint + 1);
+ continue;
+ }
+
+ /* hey, we found a matching range */
+ Assert(false);
+ }
}
}
- /* and values in the unsorted part must not be in sorted part */
- for (i = ranges->nsorted; i < ranges->nvalues; i++)
+ /* and values in the unsorted part must not be in the sorted part */
+ if (ranges->nsorted > 0)
{
compare_context cxt;
- Datum value = ranges->values[2 * ranges->nranges + i];
-
- if (ranges->nsorted == 0)
- break;
cxt.colloid = ranges->colloid;
cxt.cmpFn = ranges->cmp;
- Assert(bsearch_arg(&value, &ranges->values[2 * ranges->nranges],
- ranges->nsorted, sizeof(Datum),
- compare_values, (void *) &cxt) == NULL);
+ for (i = ranges->nsorted; i < ranges->nvalues; i++)
+ {
+ Datum value = ranges->values[2 * ranges->nranges + i];
+
+ Assert(bsearch_arg(&value, &ranges->values[2 * ranges->nranges],
+ ranges->nsorted, sizeof(Datum),
+ compare_values, (void *) &cxt) == NULL);
+ }
}
#endif
}
@@ -923,8 +923,8 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
{
Datum compar;
- Datum minvalue = ranges->values[0];
- Datum maxvalue = ranges->values[2 * ranges->nranges - 1];
+ Datum minvalue;
+ Datum maxvalue;
FmgrInfo *cmpLessFn;
FmgrInfo *cmpGreaterFn;
@@ -936,6 +936,9 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
if (ranges->nranges == 0)
return false;
+ minvalue = ranges->values[0];
+ maxvalue = ranges->values[2 * ranges->nranges - 1];
+
/*
* Otherwise, need to compare the new value with boundaries of all the
* ranges. First check if it's less than the absolute minimum, which is
Em seg., 5 de set. de 2022 às 07:15, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Sat, 3 Sept 2022 at 00:37, Ranier Vilela <ranier.vf@gmail.com> wrote:
But +1 to fix this and other issues even if they would never crash.
Yeah, I don't think any of this coding would lead to a crash, but it's
pretty weird coding and we should fix it.1. Once that ranges->nranges is invariant, avoid the loop if
ranges->nranges <= 0.
This matches the current behavior.
2. Once that ranges->nsorted is invariant, avoid the loop if
ranges->nsorted <= 0.
This matches the current behavior.
3. Remove the invariant cxt from ranges->nsorted loop.
4. Avoid possible overflows when using int to store length strings.
5. Avoid possible out-of-bounds when ranges->nranges == 0.
6. Avoid overhead when using unnecessary StringInfoData to convert Datum
a to Text b.
I've ripped out #4 and #6 for now. I think we should do #6 in master
only, probably as part of a wider cleanup of StringInfo misusages.I also spent some time trying to ensure I understand this code
correctly. I was unable to work out what the nsorted field was from
just the comments. I needed to look at the code to figure it out, so I
think the comments for that field need to be improved. A few of the
others were not that clear either. I hope I've improved them in the
attached.I was also a bit confused at various other comments. e.g:
/*
* Is the value greater than the maxval? If yes, we'll recurse to the
* right side of range array.
*/
The second comment in the v3 patch, must be:
/*
* Is the value greater than the maxval? If yes, we'll recurse
* to the right side of the range array.
*/
I think this is copy-and-paste thinko with the word "minval".
I don't see any sort of recursion going on here. All I see are
skipping of values that are out of bounds of the lower bound of the
lowest range, and above the upper bound of the highest range.
I think this kind recursion, because the loop is restarted
with:
start = (midpoint + 1);
continue;
I propose to backpatch the attached into v14 tomorrow morning (about
12 hours from now).The only other weird coding I found was in brin_range_deserialize:
for (i = 0; (i < nvalues) && (!typbyval); i++)
I imagine most compilers would optimize that so that the typbyval
check is done before the first loop and not done on every loop, but I
don't think that coding practice helps the human readers out much. I
left that one alone, for now.
Yeah, I prefer write:
if (!typbyval)
{
for (i = 0; (i < nvalues); i++)
}
regards,
Ranier Vilela
On Mon, 5 Sept 2022 at 22:15, David Rowley <dgrowleyml@gmail.com> wrote:
On Sat, 3 Sept 2022 at 00:37, Ranier Vilela <ranier.vf@gmail.com> wrote:
6. Avoid overhead when using unnecessary StringInfoData to convert Datum a to Text b.
I've ripped out #4 and #6 for now. I think we should do #6 in master
only, probably as part of a wider cleanup of StringInfo misusages.
I've attached a patch which does various other string operation cleanups.
* This changes cstring_to_text() to use cstring_to_text_with_len when
we're working with a StringInfo and can just access the .len field.
* Uses appendStringInfoString instead of appendStringInfo when there
is special formatting.
* Uses pstrdup(str) instead of psprintf("%s", str). In many cases
this will save a bit of memory
* Uses appendPQExpBufferChar instead of appendPQExpBufferStr() when
appending a 1 byte string.
* Uses appendStringInfoChar() instead of appendStringInfo() when no
formatting and string is 1 byte.
* Uses appendStringInfoChar() instead of appendStringInfoString() when
string is 1 byte.
* Uses appendPQExpBuffer(b , ...) instead of appendPQExpBufferStr(b, "%s" ...)
I'm aware there are a few other places that we could use
cstring_to_text_with_len() instead of cstring_to_text(). For example,
using the return value of snprintf() to obtain the length. I just
didn't do that because we need to take care to check the return value
isn't -1.
My grep patterns didn't account for these function calls spanning
multiple lines, so I may have missed a few.
David
Attachments:
string_fixes.patchtext/plain; charset=US-ASCII; name=string_fixes.patchDownload
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index fb72bb6cfe..6161df2790 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -1325,7 +1325,7 @@ hstore_to_json_loose(PG_FUNCTION_ARGS)
}
appendStringInfoChar(&dst, '}');
- PG_RETURN_TEXT_P(cstring_to_text(dst.data));
+ PG_RETURN_TEXT_P(cstring_to_text_with_len(dst.data, dst.len));
}
PG_FUNCTION_INFO_V1(hstore_to_json);
@@ -1370,7 +1370,7 @@ hstore_to_json(PG_FUNCTION_ARGS)
}
appendStringInfoChar(&dst, '}');
- PG_RETURN_TEXT_P(cstring_to_text(dst.data));
+ PG_RETURN_TEXT_P(cstring_to_text_with_len(dst.data, dst.len));
}
PG_FUNCTION_INFO_V1(hstore_to_jsonb);
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index a581659fe2..ea326a250b 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -3063,7 +3063,7 @@ brin_minmax_multi_summary_out(PG_FUNCTION_ARGS)
appendStringInfo(&str, "%s ... %s", a, b);
- c = cstring_to_text(str.data);
+ c = cstring_to_text_with_len(str.data, str.len);
astate_values = accumArrayResult(astate_values,
PointerGetDatum(c),
@@ -3095,15 +3095,9 @@ brin_minmax_multi_summary_out(PG_FUNCTION_ARGS)
{
Datum a;
text *b;
- StringInfoData str;
-
- initStringInfo(&str);
a = FunctionCall1(&fmgrinfo, ranges_deserialized->values[idx++]);
-
- appendStringInfoString(&str, DatumGetCString(a));
-
- b = cstring_to_text(str.data);
+ b = cstring_to_text(DatumGetCString(a));
astate_values = accumArrayResult(astate_values,
PointerGetDatum(b),
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 91ba49a14b..78abbb8196 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1062,7 +1062,7 @@ copy_table(Relation rel)
appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
}
- appendStringInfo(&cmd, ") TO STDOUT");
+ appendStringInfoString(&cmd, ") TO STDOUT");
}
else
{
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 081dfa2450..a2bdde0459 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -96,7 +96,7 @@ anytime_typmodout(bool istz, int32 typmod)
if (typmod >= 0)
return psprintf("(%d)%s", (int) typmod, tz);
else
- return psprintf("%s", tz);
+ return pstrdup(tz);
}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 6594a9aac7..2b7b1b0c0f 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1453,7 +1453,7 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
appendStringInfoChar(&buf, ')');
if (idxrec->indnullsnotdistinct)
- appendStringInfo(&buf, " NULLS NOT DISTINCT");
+ appendStringInfoString(&buf, " NULLS NOT DISTINCT");
/*
* If it has options, append "WITH (options)"
@@ -2332,9 +2332,9 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
Anum_pg_constraint_confdelsetcols, &isnull);
if (!isnull)
{
- appendStringInfo(&buf, " (");
+ appendStringInfoString(&buf, " (");
decompile_column_index_array(val, conForm->conrelid, &buf);
- appendStringInfo(&buf, ")");
+ appendStringInfoChar(&buf, ')');
}
break;
@@ -2363,7 +2363,7 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
((Form_pg_index) GETSTRUCT(indtup))->indnullsnotdistinct)
appendStringInfoString(&buf, "NULLS NOT DISTINCT ");
- appendStringInfoString(&buf, "(");
+ appendStringInfoChar(&buf, '(');
/* Fetch and build target column list */
val = SysCacheGetAttr(CONSTROID, tup,
@@ -3583,7 +3583,7 @@ pg_get_function_sqlbody(PG_FUNCTION_ARGS)
ReleaseSysCache(proctup);
- PG_RETURN_TEXT_P(cstring_to_text(buf.data));
+ PG_RETURN_TEXT_P(cstring_to_text_with_len(buf.data, buf.len));
}
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 49cdb290ac..021b760f4e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -131,7 +131,7 @@ anytimestamp_typmodout(bool istz, int32 typmod)
if (typmod >= 0)
return psprintf("(%d)%s", (int) typmod, tz);
else
- return psprintf("%s", tz);
+ return pstrdup(tz);
}
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 3cff319f02..0356e132db 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -1509,7 +1509,7 @@ append_db_pattern_cte(PQExpBuffer buf, const PatternInfoArray *pia,
have_values = true;
appendPQExpBuffer(buf, "%s\n(%d, ", comma, pattern_id);
appendStringLiteralConn(buf, info->db_regex, conn);
- appendPQExpBufferStr(buf, ")");
+ appendPQExpBufferChar(buf, ')');
comma = ",";
}
}
@@ -1765,7 +1765,7 @@ append_rel_pattern_raw_cte(PQExpBuffer buf, const PatternInfoArray *pia,
appendPQExpBufferStr(buf, ", true::BOOLEAN");
else
appendPQExpBufferStr(buf, ", false::BOOLEAN");
- appendPQExpBufferStr(buf, ")");
+ appendPQExpBufferChar(buf, ')');
comma = ",";
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d25709ad5f..f4c8b028f9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4045,7 +4045,7 @@ dumpPublication(Archive *fout, const PublicationInfo *pubinfo)
first = false;
}
- appendPQExpBufferStr(query, "'");
+ appendPQExpBufferChar(query, '\'');
if (pubinfo->pubviaroot)
appendPQExpBufferStr(query, ", publish_via_partition_root = true");
@@ -15491,7 +15491,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
appendStringLiteralAH(q, qualrelname, fout);
appendPQExpBufferStr(q, "::pg_catalog.regclass,");
appendStringLiteralAH(q, tbinfo->attnames[j], fout);
- appendPQExpBufferStr(q, ",");
+ appendPQExpBufferChar(q, ',');
appendStringLiteralAH(q, tbinfo->attmissingval[j], fout);
appendPQExpBufferStr(q, ");\n\n");
}
@@ -16361,8 +16361,8 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
}
else
{
- appendPQExpBuffer(q, "%s",
- coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
+ appendPQExpBufferStr(q,
+ coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
if (indxinfo->indnullsnotdistinct)
appendPQExpBuffer(q, " NULLS NOT DISTINCT");
appendPQExpBuffer(q, " (");
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 9aadcaad71..c749349468 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3512,8 +3512,7 @@ printVerboseErrorMessages(CState *st, pg_time_usec_t *now, bool is_retry)
resetPQExpBuffer(buf);
printfPQExpBuffer(buf, "client %d ", st->id);
- appendPQExpBuffer(buf, "%s",
- (is_retry ?
+ appendPQExpBufferStr(buf, (is_retry ?
"repeats the transaction after the error" :
"ends the failed transaction"));
appendPQExpBuffer(buf, " (try %u", st->tries);
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 61188d96f2..a141146e70 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4889,9 +4889,9 @@ pset_value_string(const char *param, printQueryOpt *popt)
else if (strcmp(param, "footer") == 0)
return pstrdup(pset_bool_string(popt->topt.default_footer));
else if (strcmp(param, "format") == 0)
- return psprintf("%s", _align2string(popt->topt.format));
+ return pstrdup(_align2string(popt->topt.format));
else if (strcmp(param, "linestyle") == 0)
- return psprintf("%s", get_line_style(&popt->topt)->name);
+ return pstrdup(get_line_style(&popt->topt)->name);
else if (strcmp(param, "null") == 0)
return pset_quoted_string(popt->nullPrint
? popt->nullPrint
Em seg., 5 de set. de 2022 às 10:40, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Mon, 5 Sept 2022 at 22:15, David Rowley <dgrowleyml@gmail.com> wrote:
On Sat, 3 Sept 2022 at 00:37, Ranier Vilela <ranier.vf@gmail.com> wrote:
6. Avoid overhead when using unnecessary StringInfoData to convert
Datum a to Text b.
I've ripped out #4 and #6 for now. I think we should do #6 in master
only, probably as part of a wider cleanup of StringInfo misusages.I've attached a patch which does various other string operation cleanups.
* This changes cstring_to_text() to use cstring_to_text_with_len when
we're working with a StringInfo and can just access the .len field.
* Uses appendStringInfoString instead of appendStringInfo when there
is special formatting.
* Uses pstrdup(str) instead of psprintf("%s", str). In many cases
this will save a bit of memory
* Uses appendPQExpBufferChar instead of appendPQExpBufferStr() when
appending a 1 byte string.
* Uses appendStringInfoChar() instead of appendStringInfo() when no
formatting and string is 1 byte.
* Uses appendStringInfoChar() instead of appendStringInfoString() when
string is 1 byte.
* Uses appendPQExpBuffer(b , ...) instead of appendPQExpBufferStr(b, "%s"
...)I'm aware there are a few other places that we could use
cstring_to_text_with_len() instead of cstring_to_text(). For example,
using the return value of snprintf() to obtain the length. I just
didn't do that because we need to take care to check the return value
isn't -1.My grep patterns didn't account for these function calls spanning
multiple lines, so I may have missed a few.
I did a search and found a few more places.
v1 attached.
regards,
Ranier Vilela
Attachments:
v1-string_fixes.patchapplication/octet-stream; name=v1-string_fixes.patchDownload
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index fb72bb6cfe..6161df2790 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -1325,7 +1325,7 @@ hstore_to_json_loose(PG_FUNCTION_ARGS)
}
appendStringInfoChar(&dst, '}');
- PG_RETURN_TEXT_P(cstring_to_text(dst.data));
+ PG_RETURN_TEXT_P(cstring_to_text_with_len(dst.data, dst.len));
}
PG_FUNCTION_INFO_V1(hstore_to_json);
@@ -1370,7 +1370,7 @@ hstore_to_json(PG_FUNCTION_ARGS)
}
appendStringInfoChar(&dst, '}');
- PG_RETURN_TEXT_P(cstring_to_text(dst.data));
+ PG_RETURN_TEXT_P(cstring_to_text_with_len(dst.data, dst.len));
}
PG_FUNCTION_INFO_V1(hstore_to_jsonb);
diff --git a/contrib/sepgsql/schema.c b/contrib/sepgsql/schema.c
index fecd02f07a..9e408cf571 100644
--- a/contrib/sepgsql/schema.c
+++ b/contrib/sepgsql/schema.c
@@ -84,7 +84,7 @@ sepgsql_schema_post_create(Oid namespaceId)
* check db_schema:{create}
*/
initStringInfo(&audit_name);
- appendStringInfo(&audit_name, "%s", quote_identifier(nsp_name));
+ appendStringInfoString(&audit_name, quote_identifier(nsp_name));
sepgsql_avc_check_perms_label(ncontext,
SEPG_CLASS_DB_SCHEMA,
SEPG_DB_SCHEMA__CREATE,
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index 6fec4853f1..3fd7185f21 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -319,9 +319,9 @@ XLogRecGetBlockRefInfo(XLogReaderState *record, bool pretty,
*fpi_len += XLogRecGetBlock(record, block_id)->bimg_len;
if (XLogRecBlockImageApply(record, block_id))
- appendStringInfo(buf, " FPW");
+ appendStringInfoString(buf, " FPW");
else
- appendStringInfo(buf, " FPW for WAL verification");
+ appendStringInfoString(buf, " FPW for WAL verification");
}
}
}
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 635d05405e..6fa9a573f0 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -347,7 +347,7 @@ filter_list_to_array(List *filterlist)
result = pstrdup(value);
for (p = result; *p; p++)
*p = pg_ascii_toupper((unsigned char) *p);
- data[i++] = PointerGetDatum(cstring_to_text(result));
+ data[i++] = PointerGetDatum(cstring_to_text_with_len(result, p - result));
pfree(result);
}
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 6c72d43beb..fd3eecf27d 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -514,7 +514,7 @@ llvm_function_reference(LLVMJitContext *context,
else if (basename != NULL)
{
/* internal function */
- funcname = psprintf("%s", basename);
+ funcname = pstrdup(basename);
}
else
{
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 1664fcee2a..e75611fdd5 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4457,7 +4457,7 @@ BackendInitialize(Port *port)
appendStringInfo(&ps_data, "%s ", port->user_name);
if (!am_walsender)
appendStringInfo(&ps_data, "%s ", port->database_name);
- appendStringInfo(&ps_data, "%s", port->remote_host);
+ appendStringInfoString(&ps_data, port->remote_host);
if (port->remote_port[0] != '\0')
appendStringInfo(&ps_data, "(%s)", port->remote_port);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 91ba49a14b..8514835ff4 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -769,7 +769,7 @@ fetch_remote_table_info(char *nspname, char *relname,
foreach(lc, MySubscription->publications)
{
if (foreach_current_index(lc) > 0)
- appendStringInfo(&pub_names, ", ");
+ appendStringInfoString(&pub_names, ", ");
appendStringInfoString(&pub_names, quote_literal_cstr(strVal(lfirst(lc))));
}
@@ -1062,7 +1062,7 @@ copy_table(Relation rel)
appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
}
- appendStringInfo(&cmd, ") TO STDOUT");
+ appendStringInfoString(&cmd, ") TO STDOUT");
}
else
{
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 495e449a9e..313947c4ec 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -1704,7 +1704,7 @@ array_dims(PG_FUNCTION_ARGS)
p += strlen(p);
}
- PG_RETURN_TEXT_P(cstring_to_text(buf));
+ PG_RETURN_TEXT_P(cstring_to_text_with_len(buf, p - buf));
}
/*
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 081dfa2450..a2bdde0459 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -96,7 +96,7 @@ anytime_typmodout(bool istz, int32 typmod)
if (typmod >= 0)
return psprintf("(%d)%s", (int) typmod, tz);
else
- return psprintf("%s", tz);
+ return pstrdup(tz);
}
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index d35b5d1f4f..55c947df9f 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -219,7 +219,7 @@ pg_tablespace_databases(PG_FUNCTION_ARGS)
}
if (tablespaceOid == DEFAULTTABLESPACE_OID)
- location = psprintf("base");
+ location = pstrdup("base");
else
location = psprintf("pg_tblspc/%u/%s", tablespaceOid,
TABLESPACE_VERSION_DIRECTORY);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 6594a9aac7..2b7b1b0c0f 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1453,7 +1453,7 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
appendStringInfoChar(&buf, ')');
if (idxrec->indnullsnotdistinct)
- appendStringInfo(&buf, " NULLS NOT DISTINCT");
+ appendStringInfoString(&buf, " NULLS NOT DISTINCT");
/*
* If it has options, append "WITH (options)"
@@ -2332,9 +2332,9 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
Anum_pg_constraint_confdelsetcols, &isnull);
if (!isnull)
{
- appendStringInfo(&buf, " (");
+ appendStringInfoString(&buf, " (");
decompile_column_index_array(val, conForm->conrelid, &buf);
- appendStringInfo(&buf, ")");
+ appendStringInfoChar(&buf, ')');
}
break;
@@ -2363,7 +2363,7 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
((Form_pg_index) GETSTRUCT(indtup))->indnullsnotdistinct)
appendStringInfoString(&buf, "NULLS NOT DISTINCT ");
- appendStringInfoString(&buf, "(");
+ appendStringInfoChar(&buf, '(');
/* Fetch and build target column list */
val = SysCacheGetAttr(CONSTROID, tup,
@@ -3583,7 +3583,7 @@ pg_get_function_sqlbody(PG_FUNCTION_ARGS)
ReleaseSysCache(proctup);
- PG_RETURN_TEXT_P(cstring_to_text(buf.data));
+ PG_RETURN_TEXT_P(cstring_to_text_with_len(buf.data, buf.len));
}
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 49cdb290ac..021b760f4e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -131,7 +131,7 @@ anytimestamp_typmodout(bool istz, int32 typmod)
if (typmod >= 0)
return psprintf("(%d)%s", (int) typmod, tz);
else
- return psprintf("%s", tz);
+ return pstrdup(tz);
}
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 3cff319f02..0356e132db 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -1509,7 +1509,7 @@ append_db_pattern_cte(PQExpBuffer buf, const PatternInfoArray *pia,
have_values = true;
appendPQExpBuffer(buf, "%s\n(%d, ", comma, pattern_id);
appendStringLiteralConn(buf, info->db_regex, conn);
- appendPQExpBufferStr(buf, ")");
+ appendPQExpBufferChar(buf, ')');
comma = ",";
}
}
@@ -1765,7 +1765,7 @@ append_rel_pattern_raw_cte(PQExpBuffer buf, const PatternInfoArray *pia,
appendPQExpBufferStr(buf, ", true::BOOLEAN");
else
appendPQExpBufferStr(buf, ", false::BOOLEAN");
- appendPQExpBufferStr(buf, ")");
+ appendPQExpBufferChar(buf, ')');
comma = ",";
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d25709ad5f..e1e63cad91 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2845,14 +2845,14 @@ dumpDatabase(Archive *fout)
"datacl, acldefault('d', datdba) AS acldefault, "
"datistemplate, datconnlimit, ");
if (fout->remoteVersion >= 90300)
- appendPQExpBuffer(dbQry, "datminmxid, ");
+ appendPQExpBufferStr(dbQry, "datminmxid, ");
else
- appendPQExpBuffer(dbQry, "0 AS datminmxid, ");
+ appendPQExpBufferStr(dbQry, "0 AS datminmxid, ");
if (fout->remoteVersion >= 150000)
- appendPQExpBuffer(dbQry, "datlocprovider, daticulocale, datcollversion, ");
+ appendPQExpBufferStr(dbQry, "datlocprovider, daticulocale, datcollversion, ");
else
- appendPQExpBuffer(dbQry, "'c' AS datlocprovider, NULL AS daticulocale, NULL AS datcollversion, ");
- appendPQExpBuffer(dbQry,
+ appendPQExpBufferStr(dbQry, "'c' AS datlocprovider, NULL AS daticulocale, NULL AS datcollversion, ");
+ appendPQExpBufferStr(dbQry,
"(SELECT spcname FROM pg_tablespace t WHERE t.oid = dattablespace) AS tablespace, "
"shobj_description(oid, 'pg_database') AS description "
"FROM pg_database "
@@ -3685,9 +3685,9 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
printfPQExpBuffer(query,
"SELECT pol.oid, pol.tableoid, pol.polrelid, pol.polname, pol.polcmd, ");
if (fout->remoteVersion >= 100000)
- appendPQExpBuffer(query, "pol.polpermissive, ");
+ appendPQExpBufferStr(query, "pol.polpermissive, ");
else
- appendPQExpBuffer(query, "'t' as polpermissive, ");
+ appendPQExpBufferStr(query, "'t' as polpermissive, ");
appendPQExpBuffer(query,
"CASE WHEN pol.polroles = '{0}' THEN NULL ELSE "
" pg_catalog.array_to_string(ARRAY(SELECT pg_catalog.quote_ident(rolname) from pg_catalog.pg_roles WHERE oid = ANY(pol.polroles)), ', ') END AS polroles, "
@@ -4045,7 +4045,7 @@ dumpPublication(Archive *fout, const PublicationInfo *pubinfo)
first = false;
}
- appendPQExpBufferStr(query, "'");
+ appendPQExpBufferChar(query, '\'');
if (pubinfo->pubviaroot)
appendPQExpBufferStr(query, ", publish_via_partition_root = true");
@@ -5477,7 +5477,7 @@ getCollations(Archive *fout, int *numCollations)
* system-defined collations at dump-out time.
*/
- appendPQExpBuffer(query, "SELECT tableoid, oid, collname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, collname, "
"collnamespace, "
"collowner "
"FROM pg_collation");
@@ -5545,7 +5545,7 @@ getConversions(Archive *fout, int *numConversions)
* system-defined conversions at dump-out time.
*/
- appendPQExpBuffer(query, "SELECT tableoid, oid, conname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, conname, "
"connamespace, "
"conowner "
"FROM pg_conversion");
@@ -5682,7 +5682,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
* system-defined opclasses at dump-out time.
*/
- appendPQExpBuffer(query, "SELECT tableoid, oid, opcname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, opcname, "
"opcnamespace, "
"opcowner "
"FROM pg_opclass");
@@ -5750,7 +5750,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
* system-defined opfamilies at dump-out time.
*/
- appendPQExpBuffer(query, "SELECT tableoid, oid, opfname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, opfname, "
"opfnamespace, "
"opfowner "
"FROM pg_opfamily");
@@ -5856,7 +5856,7 @@ getAggregates(Archive *fout, int *numAggs)
}
else
{
- appendPQExpBuffer(query, "SELECT tableoid, oid, proname AS aggname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, proname AS aggname, "
"pronamespace AS aggnamespace, "
"pronargs, proargtypes, "
"proowner, "
@@ -6205,7 +6205,7 @@ getTables(Archive *fout, int *numTables)
* wrong answers if any concurrent DDL is happening.
*/
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"SELECT c.tableoid, c.oid, c.relname, "
"c.relnamespace, c.relkind, c.reltype, "
"c.relowner, "
@@ -6732,7 +6732,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
}
appendPQExpBufferChar(tbloids, '}');
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"SELECT t.tableoid, t.oid, i.indrelid, "
"t.relname AS indexname, "
"pg_catalog.pg_get_indexdef(i.indexrelid) AS indexdef, "
@@ -6747,14 +6747,14 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
if (fout->remoteVersion >= 90400)
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"i.indisreplident, ");
else
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"false AS indisreplident, ");
if (fout->remoteVersion >= 110000)
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"inh.inhparent AS parentidx, "
"i.indnkeyatts AS indnkeyatts, "
"i.indnatts AS indnatts, "
@@ -6767,7 +6767,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
" WHERE attrelid = i.indexrelid AND "
" attstattarget >= 0) AS indstatvals, ");
else
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"0 AS parentidx, "
"i.indnatts AS indnkeyatts, "
"i.indnatts AS indnatts, "
@@ -6775,10 +6775,10 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"'' AS indstatvals, ");
if (fout->remoteVersion >= 150000)
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"i.indnullsnotdistinct ");
else
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"false AS indnullsnotdistinct ");
/*
@@ -7002,11 +7002,11 @@ getExtendedStatistics(Archive *fout)
query = createPQExpBuffer();
if (fout->remoteVersion < 130000)
- appendPQExpBuffer(query, "SELECT tableoid, oid, stxname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
"stxnamespace, stxowner, (-1) AS stxstattarget "
"FROM pg_catalog.pg_statistic_ext");
else
- appendPQExpBuffer(query, "SELECT tableoid, oid, stxname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
"stxnamespace, stxowner, stxstattarget "
"FROM pg_catalog.pg_statistic_ext");
@@ -7729,7 +7729,7 @@ getEventTriggers(Archive *fout, int *numEventTriggers)
query = createPQExpBuffer();
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"SELECT e.tableoid, e.oid, evtname, evtenabled, "
"evtevent, evtowner, "
"array_to_string(array("
@@ -7809,7 +7809,7 @@ getProcLangs(Archive *fout, int *numProcLangs)
int i_acldefault;
int i_lanowner;
- appendPQExpBuffer(query, "SELECT tableoid, oid, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, "
"lanname, lanpltrusted, lanplcallfoid, "
"laninline, lanvalidator, "
"lanacl, "
@@ -8768,7 +8768,7 @@ getTSDictionaries(Archive *fout, int *numTSDicts)
query = createPQExpBuffer();
- appendPQExpBuffer(query, "SELECT tableoid, oid, dictname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, dictname, "
"dictnamespace, dictowner, "
"dicttemplate, dictinitoption "
"FROM pg_ts_dict");
@@ -8904,7 +8904,7 @@ getTSConfigurations(Archive *fout, int *numTSConfigs)
query = createPQExpBuffer();
- appendPQExpBuffer(query, "SELECT tableoid, oid, cfgname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, cfgname, "
"cfgnamespace, cfgowner, cfgparser "
"FROM pg_ts_config");
@@ -8972,7 +8972,7 @@ getForeignDataWrappers(Archive *fout, int *numForeignDataWrappers)
query = createPQExpBuffer();
- appendPQExpBuffer(query, "SELECT tableoid, oid, fdwname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, fdwname, "
"fdwowner, "
"fdwhandler::pg_catalog.regproc, "
"fdwvalidator::pg_catalog.regproc, "
@@ -9063,7 +9063,7 @@ getForeignServers(Archive *fout, int *numForeignServers)
query = createPQExpBuffer();
- appendPQExpBuffer(query, "SELECT tableoid, oid, srvname, "
+ appendPQExpBufferStr(query, "SELECT tableoid, oid, srvname, "
"srvowner, "
"srvfdw, srvtype, srvversion, srvacl, "
"acldefault('S', srvowner) AS acldefault, "
@@ -9167,7 +9167,7 @@ getDefaultACLs(Archive *fout, int *numDefaultACLs)
* for the case of 'S' (DEFACLOBJ_SEQUENCE) which must be converted to
* 's'.
*/
- appendPQExpBuffer(query,
+ appendPQExpBufferStr(query,
"SELECT oid, tableoid, "
"defaclrole, "
"defaclnamespace, "
@@ -15491,7 +15491,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
appendStringLiteralAH(q, qualrelname, fout);
appendPQExpBufferStr(q, "::pg_catalog.regclass,");
appendStringLiteralAH(q, tbinfo->attnames[j], fout);
- appendPQExpBufferStr(q, ",");
+ appendPQExpBufferChar(q, ',');
appendStringLiteralAH(q, tbinfo->attmissingval[j], fout);
appendPQExpBufferStr(q, ");\n\n");
}
@@ -16361,11 +16361,11 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
}
else
{
- appendPQExpBuffer(q, "%s",
- coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
+ appendPQExpBufferStr(q,
+ coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
if (indxinfo->indnullsnotdistinct)
- appendPQExpBuffer(q, " NULLS NOT DISTINCT");
- appendPQExpBuffer(q, " (");
+ appendPQExpBufferStr(q, " NULLS NOT DISTINCT");
+ appendPQExpBufferStr(q, " (");
for (k = 0; k < indxinfo->indnkeyattrs; k++)
{
int indkey = (int) indxinfo->indkeys[k];
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index d665b257c9..69ae027bd3 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -978,7 +978,7 @@ dumpRoleMembership(PGconn *conn)
"ug.rolname AS grantor, "
"a.admin_option");
if (dump_inherit_option)
- appendPQExpBuffer(buf, ", a.inherit_option");
+ appendPQExpBufferStr(buf, ", a.inherit_option");
appendPQExpBuffer(buf, " FROM pg_auth_members a "
"LEFT JOIN %s ur on ur.oid = a.roleid "
"LEFT JOIN %s um on um.oid = a.member "
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 9aadcaad71..c749349468 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3512,8 +3512,7 @@ printVerboseErrorMessages(CState *st, pg_time_usec_t *now, bool is_retry)
resetPQExpBuffer(buf);
printfPQExpBuffer(buf, "client %d ", st->id);
- appendPQExpBuffer(buf, "%s",
- (is_retry ?
+ appendPQExpBufferStr(buf, (is_retry ?
"repeats the transaction after the error" :
"ends the failed transaction"));
appendPQExpBuffer(buf, " (try %u", st->tries);
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 61188d96f2..a141146e70 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4889,9 +4889,9 @@ pset_value_string(const char *param, printQueryOpt *popt)
else if (strcmp(param, "footer") == 0)
return pstrdup(pset_bool_string(popt->topt.default_footer));
else if (strcmp(param, "format") == 0)
- return psprintf("%s", _align2string(popt->topt.format));
+ return pstrdup(_align2string(popt->topt.format));
else if (strcmp(param, "linestyle") == 0)
- return psprintf("%s", get_line_style(&popt->topt)->name);
+ return pstrdup(get_line_style(&popt->topt)->name);
else if (strcmp(param, "null") == 0)
return pset_quoted_string(popt->nullPrint
? popt->nullPrint
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 327a69487b..153b095e6b 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2148,7 +2148,7 @@ describeOneTableDetails(const char *schemaname,
"SELECT inhparent::pg_catalog.regclass,\n"
" pg_catalog.pg_get_expr(c.relpartbound, c.oid),\n ");
- appendPQExpBuffer(&buf,
+ appendPQExpBufferStr(&buf,
pset.sversion >= 140000 ? "inhdetachpending" :
"false as inhdetachpending");
@@ -2311,7 +2311,7 @@ describeOneTableDetails(const char *schemaname,
printfPQExpBuffer(&tmpbuf, _("unique"));
if (strcmp(indnullsnotdistinct, "t") == 0)
appendPQExpBufferStr(&tmpbuf, _(" nulls not distinct"));
- appendPQExpBuffer(&tmpbuf, _(", "));
+ appendPQExpBufferStr(&tmpbuf, _(", "));
}
else
resetPQExpBuffer(&tmpbuf);On Tue, 6 Sept 2022 at 06:07, Ranier Vilela <ranier.vf@gmail.com> wrote:
I did a search and found a few more places.
v1 attached.
Thanks. I've done a bit more looking and found a few more places that
we can improve and I've pushed the result.
It feels like it would be good if we had a way to detect a few of
these issues much earlier than we are currently. There's been a long
series of commits fixing up this sort of thing. If we had a tool to
parse the .c files and look for things like a function call to
appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
no va_arg arguments).
I'll hold off a few days before pushing the other patch. Tom stamped
beta4 earlier, so I'll hold off until after the tag.
David
Em seg., 5 de set. de 2022 às 22:29, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Tue, 6 Sept 2022 at 06:07, Ranier Vilela <ranier.vf@gmail.com> wrote:
I did a search and found a few more places.
v1 attached.Thanks. I've done a bit more looking and found a few more places that
we can improve and I've pushed the result.
Thanks.
It feels like it would be good if we had a way to detect a few of
these issues much earlier than we are currently. There's been a long
series of commits fixing up this sort of thing. If we had a tool to
parse the .c files and look for things like a function call to
appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
no va_arg arguments).
StaticAssert could check va_arg no?
regards,
Ranier Vilela
On Tue, 6 Sept 2022 at 13:52, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em seg., 5 de set. de 2022 às 22:29, David Rowley <dgrowleyml@gmail.com> escreveu:
It feels like it would be good if we had a way to detect a few of
these issues much earlier than we are currently. There's been a long
series of commits fixing up this sort of thing. If we had a tool to
parse the .c files and look for things like a function call to
appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
no va_arg arguments).StaticAssert could check va_arg no?
I'm not sure exactly what you have in mind. If you think you have a
way to make that work, it would be good to see a patch with it.
David
Em seg., 5 de set. de 2022 às 23:02, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Tue, 6 Sept 2022 at 13:52, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em seg., 5 de set. de 2022 às 22:29, David Rowley <dgrowleyml@gmail.com>
escreveu:
It feels like it would be good if we had a way to detect a few of
these issues much earlier than we are currently. There's been a long
series of commits fixing up this sort of thing. If we had a tool to
parse the .c files and look for things like a function call to
appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
no va_arg arguments).StaticAssert could check va_arg no?
I'm not sure exactly what you have in mind. If you think you have a
way to make that work, it would be good to see a patch with it.
I will study the matter.
But first, I would like to continue with this correction of using strings.
In the following cases:
fprintf -> fputs -> fputc
printf -> puts -> putchar
There are many occurrences, do you think it would be worth the effort?
regards,
Ranier Vilela
On Tue, 6 Sept 2022 at 23:25, Ranier Vilela <ranier.vf@gmail.com> wrote:
But first, I would like to continue with this correction of using strings.
In the following cases:
fprintf -> fputs -> fputc
printf -> puts -> putcharThere are many occurrences, do you think it would be worth the effort?
I'm pretty unexcited about that. Quite a bit of churn and adding
another precedent that we currently have no good way to enforce or
maintain.
In addition to that, puts() is a fairly seldom used function, which
perhaps is because it's a bit quirky and appends a \n to the end of
the string. I'm just imagining all the bugs where we append an extra
newline. But, feel free to open another thread about it and see if you
can drum up any support.
David
On Tue, 6 Sept 2022 at 13:29, David Rowley <dgrowleyml@gmail.com> wrote:
I'll hold off a few days before pushing the other patch. Tom stamped
beta4 earlier, so I'll hold off until after the tag.
I've now pushed this.
David
Em seg., 12 de set. de 2022 às 20:06, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Tue, 6 Sept 2022 at 13:29, David Rowley <dgrowleyml@gmail.com> wrote:
I'll hold off a few days before pushing the other patch. Tom stamped
beta4 earlier, so I'll hold off until after the tag.I've now pushed this.
Thank you David.
But the correct thing is to put you also as author, after all, there's more
of your code there than mine.
Anyway, I appreciate the consideration.
regards,
Ranier Vilela
Em seg., 5 de set. de 2022 às 23:02, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Tue, 6 Sept 2022 at 13:52, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em seg., 5 de set. de 2022 às 22:29, David Rowley <dgrowleyml@gmail.com>
escreveu:
It feels like it would be good if we had a way to detect a few of
these issues much earlier than we are currently. There's been a long
series of commits fixing up this sort of thing. If we had a tool to
parse the .c files and look for things like a function call to
appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
no va_arg arguments).StaticAssert could check va_arg no?
I'm not sure exactly what you have in mind. If you think you have a
way to make that work, it would be good to see a patch with it.
About this:
1. StaticAssertSmt can not help.
Although some posts on the web show that it is possible to calculate the
number of arguments,
I didn't get anything useful.
So I left this option.
2. Compiler supports
Best solution.
But currently does not allow the suggestion to use another function.
3. Owner tool
Temporary solution.
Can help, until the compilers build support for it.
So, I made one very simple tool, can do the basics here.
Not meant to be some universal lint.
It only processes previously coded functions.
pg_check test1.c
line (1): should be appendPQExpBufferStr?
line (2): should be appendPQExpBufferChar?
line (4): should be appendPQExpBufferStr?
line (5): should be appendPQExpBufferStr?
I don't think it's anywhere near the quality to be considered Postgres, but
it could be a start.
If it helps, great, if not, fine.
regards,
Ranier Vilela