Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options
On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera wrote: > > > The latest patch sent by Bharath looks good to me. Would you like to > > commit it or shall I take care of it? > > Please, go ahead. > Okay, I'll push it early next week (by Tuesday) unless there are more comments or suggestions. Thanks! -- With Regards, Amit Kapila.
Re: row filtering for logical replication
Hi.
I have been looking at the latest patch set (v16). Below are my review
comments and some patches.
The patches are:
v16-0001. This is identical to your previously posted 0001 patch. (I
only attached it again hoping it can allow the cfbot to keep working
OK).
v16-0002,0003. These are for demonstrating some of the review comments
v16-0004. This is a POC plan cache for your consideration.
//
REVIEW COMMENTS
===
1. Patch 0001 comment - typo
you can optionally filter rows that does not satisfy a WHERE condition
typo: does/does
~~
2. Patch 0001 comment - typo
The WHERE clause should probably contain only columns that are part of
the primary key or that are covered by REPLICA IDENTITY. Otherwise,
and DELETEs won't be replicated.
typo: "Otherwise, and DELETEs" ??
~~
3. Patch 0001 comment - typo and clarification
If your publication contains partitioned table, the parameter
publish_via_partition_root determines if it uses the partition row filter (if
the parameter is false -- default) or the partitioned table row filter.
Typo: "contains partitioned table" -> "contains a partitioned table"
Also, perhaps the text "or the partitioned table row filter." should
say "or the root partitioned table row filter." to disambiguate the
case where there are more levels of partitions like A->B->C. e.g. What
filter does C use?
~~
4. src/backend/catalog/pg_publication.c - misleading names
-publication_add_relation(Oid pubid, Relation targetrel,
+publication_add_relation(Oid pubid, PublicationRelationInfo *targetrel,
bool if_not_exists)
Leaving this parameter name as "targetrel" seems a bit misleading now
in the function code. Maybe this should be called something like "pri"
which is consistent with other places where you have declared
PublicationRelationInfo.
Also, consider declaring some local variables so that the patch may
have less impact on existing code. e.g.
Oid relid = pri->relid
Relation *targetrel = relationinfo->relation
~~
5. src/backend/commands/publicationcmds.c - simplify code
- rels = OpenTableList(stmt->tables);
+ if (stmt->tableAction == DEFELEM_DROP)
+ rels = OpenTableList(stmt->tables, true);
+ else
+ rels = OpenTableList(stmt->tables, false);
Consider writing that code more simply as just:
rels = OpenTableList(stmt->tables, stmt->tableAction == DEFELEM_DROP);
~~
6. src/backend/commands/publicationcmds.c - bug?
- CloseTableList(rels);
+ CloseTableList(rels, false);
}
Is this a potential bug? When you called OpenTableList the 2nd param
was maybe true/false, so is it correct to be unconditionally false
here? I am not sure.
~~
7. src/backend/commands/publicationcmds.c - OpenTableList function comment.
* Open relations specified by a RangeVar list.
+ * AlterPublicationStmt->tables has a different list element, hence, is_drop
+ * indicates if it has a RangeVar (true) or PublicationTable (false).
* The returned tables are locked in ShareUpdateExclusiveLock mode in order to
* add them to a publication.
I am not sure about this. Should that comment instead say "indicates
if it has a Relation (true) or PublicationTable (false)"?
~~
8. src/backend/commands/publicationcmds.c - OpenTableList
- RangeVar *rv = castNode(RangeVar, lfirst(lc));
- bool recurse = rv->inh;
+ PublicationTable *t = NULL;
+ RangeVar *rv;
+ bool recurse;
Relation rel;
Oid myrelid;
+ if (is_drop)
+ {
+ rv = castNode(RangeVar, lfirst(lc));
+ }
+ else
+ {
+ t = lfirst(lc);
+ rv = castNode(RangeVar, t->relation);
+ }
+
+ recurse = rv->inh;
+
For some reason it feels kind of clunky to me for this function to be
processing the list differently according to the 2nd param. e.g. the
name "is_drop" seems quite unrelated to the function code, and more to
do with where it was called from. Sorry, I don't have any better ideas
for improvement atm.
~~
9. src/backend/commands/publicationcmds.c - OpenTableList bug?
- rels = lappend(rels, rel);
+ pri = palloc(sizeof(PublicationRelationInfo));
+ pri->relid = myrelid;
+ pri->relation = rel;
+ if (!is_drop)
+ pri->whereClause = t->whereClause;
+ rels = lappend(rels, pri);
I felt maybe this is a possible bug here because there seems no code
explicitly assigning the whereClause = NULL if "is_drop" is true so
maybe it can have a garbage value which could cause problems later.
Maybe this is fixed by using palloc0.
Same thing is 2x in this function.
~~
10. src/backend/commands/publicationcmds.c - CloseTableList function comment
@@ -587,16 +609,28 @@ OpenTableList(List *tables)
* Close all relations in the list.
*/
static void
-CloseTableList(List *rels)
+CloseTableList(List *rels, bool is_drop)
{
Probably the meaning of "is_drop" should be described in this function comment.
~~
11. src/backend/replication/pgoutput/pgoutput.c - get_rel_sync_entry signature.
-static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data, Oid relid);
+static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data, Relation rel);
I see that this fu
Re: Teach pg_receivewal to use lz4 compression
‐‐‐ Original Message ‐‐‐ On Friday, July 2nd, 2021 at 03:10, Michael Paquier wrote: > On Thu, Jul 01, 2021 at 02:10:17PM +, gkokola...@pm.me wrote: > > > Micheal suggested on the same thread to move my entry in the help output so > > that > > > > the output remains ordered. I would like the options for the compression > > method and > > > > the already existing compression level to next to each other if possible. > > Then it > > > > should be either 'X' or 'Y'. > > Hmm. Grouping these together makes sense for the user. One choice > > that we have here is to drop the short option, and just use a long > > one. What I think is important for the user when it comes to this > > option is the consistency of its naming across all the tools > > supporting it. pg_dump and pg_basebackup, where we could plug LZ4, > > already use most of the short options you could use for pg_receivewal, > > having only a long one gives a bit more flexibility. Good point. I am going with that one. > -- > > Michael
Re: Add proper planner support for ORDER BY / DISTINCT aggregates
> > This allows us to give presorted input to both aggregates in the following > case: > > SELECT agg(a ORDER BY a),agg2(a ORDER BY a,b) ... > > but just the first agg in this one: > > SELECT agg(a ORDER BY a),agg2(a ORDER BY c) ... I don't know if it's acceptable, but in the case where you add both an aggregate with an ORDER BY clause, and another aggregate without the clause, the output for the unordered one will change and use the same ordering, maybe suprising the unsuspecting user. Would that be acceptable ? > When testing the performance of all this I found that when a suitable > index exists to provide pre-sorted input for the aggregation that the > performance does improve. Unfortunately, it looks like things get more > complex when no index exists. In this case, since we're setting > pathkeys to tell the planner we need a plan that provides pre-sorted > input to the aggregates, the planner will add a sort below the > aggregate node. I initially didn't see any problem with that as it > just moves the sort to a Sort node rather than having it done > implicitly inside nodeAgg.c. The problem is, it just does not perform > as well. I guess this is because when the sort is done inside > nodeAgg.c that the transition function is called in a tight loop while > reading records back from the tuplestore. In the patched version, > there's an additional node transition in between nodeAgg and nodeSort > and that causes slower performance. For now, I'm not quite sure what > to do about that. We set the plan pathkeys well before we could > possibly decide if asking for pre-sorted input for the aggregates > would be a good idea or not. I was curious about the performance implication of that additional transition, and could not reproduce a signifcant difference. I may be doing something wrong: how did you highlight it ? Regards, -- Ronan Dunklau
Re: rand48 replacement
On Thu, 1 Jul 2021 at 22:18, Fabien COELHO wrote: > > > However, this patch seems to be replacing that with a simple > > modulo operation, which is perhaps the worst possible way to do it. > > The modulo operation is biased for large ranges close to the limit, sure. > Also, the bias is somehow of the same magnitude as the FP multiplication > approach used previously, so the "worst" has not changed much, it is > really the same as before. > It may be true that the bias is of the same magnitude as FP multiply, but it is not of the same nature. With FP multiply, the more-likely-to-be-chosen values are more-or-less evenly distributed across the range, whereas modulo concentrates them all at one end, making it more likely to bias test results. It's worth paying attention to how other languages/libraries implement this, and basically no one chooses the modulo method, which ought to raise alarm bells. Of the biased methods, it has the worst kind of bias and the worst performance. If a biased method is OK, then the biased integer multiply method seems to be the clear winner. This requires the high part of a 64x64-bit product, which is trivial if 128-bit integers are available, but would need a little more work otherwise. There's some code in common/d2s that might be suitable. Most other implementations tend to use an unbiased method though, and I think it's worth doing the same. It might be a bit slower, or even faster depending on implementation and platform, but in the context of the DB as a whole, I don't think a few extra cycles matters either way. The method recommended at the very end of that blog seems to be pretty good all round, but any of the other commonly used unbiased methods would probably be OK too. Regards, Dean
Re: Add proper planner support for ORDER BY / DISTINCT aggregates
On Fri, 2 Jul 2021 at 19:54, Ronan Dunklau wrote: > I don't know if it's acceptable, but in the case where you add both an > aggregate with an ORDER BY clause, and another aggregate without the clause, > the output for the unordered one will change and use the same ordering, maybe > suprising the unsuspecting user. Would that be acceptable ? That's a good question. There was an argument in [1] that mentions that there might be a group of people who rely on aggregation being done in a certain order where they're not specifying an ORDER BY clause in the aggregate. If that group of people exists, then it's possible they might be upset in the scenario that you describe. I also think that it's going to be pretty hard to make significant gains in this area if we are too scared to make changes to undefined behaviour. You wouldn't have to look too hard in the pgsql-general mailing list to find someone complaining that their query output is in the wrong order on some query that does not have an ORDER BY. We pretty much always tell those people that the order is undefined without an ORDER BY. I'm not too sure why Tom in [1] classes the ORDER BY aggregate people any differently. We'll be stuck forever here and in many other areas if we're too scared to change the order of aggregation. You could argue that something like parallelism has changed that for people already. I think the multi-batch Hash Aggregate code could also change this. > I was curious about the performance implication of that additional transition, > and could not reproduce a signifcant difference. I may be doing something > wrong: how did you highlight it ? It was pretty basic. I just created a table with two columns and no index and did something like SELECT a,SUM(b ORDER BY b) from ab GROUP BY 1; the new code will include a Sort due to lack of any index and the old code would have done a sort inside nodeAgg.c. I imagine that the overhead comes from the fact that in the patched version nodeAgg.c must ask its subnode (nodeSort.c) for the next tuple each time, whereas unpatched nodeAgg.c already has all the tuples in a tuplestore and can fetch them very quickly in a tight loop. David [1] https://www.postgresql.org/message-id/6538.1522096067%40sss.pgh.pa.us
Re: Commit fest manager
On Fri, 2 Jul 2021 at 15:04, vignesh C wrote: > I'm interested in assisting Ibrar Ahmed. It might be worth talking to Ibrar to see where you can lend a hand. I think in terms of the number of patches, this might be our biggest commitfest yet. 2020-07 246 2020-09 235 2020-11 244 2021-01 260 2020-03 295 2020-07 342 It's possible Ibrar would welcome you helping to take care of some of the duties. I've never been brave enough to take on the CF manager role yet, but from what I can see, to do a good job takes a huge amount of effort. David
Re: Numeric multiplication overflow errors
On Fri, 2 Jul 2021 at 00:28, Dean Rasheed wrote: > > On Thu, 1 Jul 2021 at 06:43, David Rowley wrote: > > > > Master @ 3788c6678 > > > > Execution Time: 8306.319 ms > > Execution Time: 8407.785 ms > > Execution Time: 8491.056 ms > > > > Master + numeric-agg-sumX2-overflow-v2.patch > > Execution Time: 6633.278 ms > > Execution Time: 6657.350 ms > > Execution Time: 6568.184 ms > > > > Hmm, I'm a bit surprised by those numbers. I wouldn't have expected it > to be spending enough time in the serialization/deserialization code > for it to make such a difference. I was only able to measure a 2-3% > performance improvement with the same test, and that was barely above > the noise. I ran this again with a few different worker counts after tuning a few memory settings so there was no spilling to disk and so everything was in RAM. Mostly so I could get consistent results. Here's the results. Average over 3 runs on each: Workers Master Patched Percent 811094.1 11084.9 100.08% 16 8711.4 8562.6 101.74% 32 6961.4 6726.3 103.50% 64 6137.4 5854.8 104.83% 128 6090.3 5747.4 105.96% So the gains are much less at lower worker counts. I think this is because most of the gains are in the serial part of the plan and with higher worker counts that part of the plan is relatively much bigger. So likely performance isn't too critical here, but it is something to keep in mind. David
Re: Added schema level support for publication.
On Fri, Jul 2, 2021 at 10:18 AM tanghy.f...@fujitsu.com wrote: > > On Wednesday, June 30, 2021 7:43 PM vignesh C wrote: > > > > Thanks for reporting this issue, the attached v9 patch fixes this issue. > > This also fixes the other issue you reported at [1]. > > A comment on v9: > > src/bin/psql/describe.c > > + if (pset.sversion >= 15000) > > I think it should be 15. Thanks for reporting this, I will fix this in the next version of the patch. Regards, Vignesh
Re: Signed vs. Unsigned (some)
On 16.06.21 10:48, Peter Eisentraut wrote: On 15.06.21 10:17, Kyotaro Horiguchi wrote: The definitions are not ((type) -1) but ((type) 0x) so actually they might be different if we forget to widen the constant when widening the types. Regarding to the compiler behavior, I think we are assuming C99[1] and C99 defines that -1 is converted to Uxxx_MAX. (6.3.1.3 Singed and unsigned integers) I'm +0.2 on it. It might be worthwhile as a matter of style. I think since we have the constants we should use them. I have pushed the InvalidBucket changes. The use of InvalidBlockNumber with vac_update_relstats() looks a bit fishy to me. We are using in the same call 0 as the default for num_all_visible_pages, and we generally elsewhere also use 0 as the starting value for relpages, so it's not clear to me why it should be -1 or InvalidBlockNumber here. I'd rather leave it "slightly wrong" for now so it can be checked again.
RE: [HACKERS] logical decoding of two-phase transactions
On Thursday, July 1, 2021 11:48 AM Ajin Cherian > > Adding a new patch (0004) to this patch-set that handles skipping of > empty streamed transactions. patch-0003 did not > handle empty streamed transactions. To support this, added a new flag > "sent_stream_start" to PGOutputTxnData. > Also transactions which do not have any data will not be stream > committed or stream prepared or stream aborted. > Do review and let me know if you have any comments. > Thanks for your patch. I met an issue while using it. When a transaction contains TRUNCATE, the subscriber reported an error: " ERROR: no data left in message" and the data couldn't be replicated. Steps to reproduce the issue: (set logical_decoding_work_mem to 64kB at publisher so that streaming could work. ) --publisher-- create table test (a int primary key, b varchar); create publication pub for table test; --subscriber-- create table test (a int primary key, b varchar); create subscription sub connection 'dbname=postgres' publication pub with(two_phase=on, streaming=on); --publisher-- BEGIN; TRUNCATE test; INSERT INTO test SELECT i, md5(i::text) FROM generate_series(1001, 6000) s(i); UPDATE test SET b = md5(b) WHERE mod(a,2) = 0; DELETE FROM test WHERE mod(a,3) = 0; COMMIT; The above case worked ok when remove 0004 patch, so I think it’s a problem of 0004 patch. Please have a look. Regards Tang
Re: Small clean up in nodeAgg.c
On Thu, 1 Jul 2021 at 11:09, Tom Lane wrote: > Just reading it over quickly, I noticed a comment mentioning > "aggcombinedfn" which I suppose should be "aggcombinefn". Thanks. I've fixed that locally. > No particular opinion on whether this is a net reduction > of logical complexity. I had another look over it and I think we do need to be more clear about when we're talking about aggtransfn and aggcombinefn. The existing code uses variables name aggtransfn when the value stored could be the value for the aggcombinefn. Additionally, the other change to remove the special case build_aggregate_combinefn_expr() function seems good in a sense of reusing more code and reducing the amount of code in that file. Unless anyone thinks differently about this, I plan on pushing the patch in the next day or so. David
Re: wrong relkind error messages
On 02.07.21 08:25, Michael Paquier wrote:
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("ALTER action %s cannot be performed on relation
\"%s\"",
+ action_str, RelationGetRelationName(rel)),
+errdetail_relkind_not_supported(rel->rd_rel->relkind)));
Perhaps the result of alter_table_type_to_string() is worth a note for
translators?
ok
+ case AT_DetachPartitionFinalize:
+ return "DETACH PARTITION FINALIZE";
To be exact, I think that this one should be "DETACH PARTITION
... FINALIZE".
ok
+ if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("cannot change schema of index \"%s\"",
+ rv->relname),
+errhint("Change the schema of the table instead.")));
+ else if (relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("cannot change schema of composite type
\"%s\"",
+ rv->relname),
+errhint("Use ALTER TYPE instead.")));
I would simplify this part by removing the errhint(), and use "cannot
change schema of relation .." as error string, with a dose of
errdetail_relkind_not_supported().
I aimed for parity with the error reporting in ATExecChangeOwner() here.
+errmsg("relation \"%s\" cannot have triggers",
+ RelationGetRelationName(rel)),
Better as "cannot create/rename/remove triggers on relation \"%s\""
for the three code paths of trigger.c?
+errmsg("relation \"%s\" cannot have rules",
[...]
+errmsg("relation \"%s\" cannot have rules",
For rewriteDefine.c, this could be "cannot create/rename rules on
relation".
I had it like that, but in previous reviews some people liked it better
this way. ;-) I tend to agree with that, since the error condition
isn't that you can't create a rule/etc. (like, due to incorrect
prerequisite state) but that there cannot be one ever.
Re: Numeric multiplication overflow errors
On Fri, 2 Jul 2021 at 10:24, David Rowley wrote:
>
> I ran this again with a few different worker counts after tuning a few
> memory settings so there was no spilling to disk and so everything was
> in RAM. Mostly so I could get consistent results.
>
> Here's the results. Average over 3 runs on each:
>
> Workers Master Patched Percent
> 811094.1 11084.9 100.08%
> 16 8711.4 8562.6 101.74%
> 32 6961.4 6726.3 103.50%
> 64 6137.4 5854.8 104.83%
> 128 6090.3 5747.4 105.96%
>
Thanks for testing again. Those are nice looking results, and are much
more in line with what I was seeing.
> So the gains are much less at lower worker counts. I think this is
> because most of the gains are in the serial part of the plan and with
> higher worker counts that part of the plan is relatively much bigger.
>
> So likely performance isn't too critical here, but it is something to
> keep in mind.
>
Yes, agreed. I suspect there's not much more that can be shaved off
this particular piece of code now though. Here's an update with the
last set of changes discussed.
Regards,
Dean
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
new file mode 100644
index eb78f0b..a8c6bbf
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -515,6 +515,9 @@ static void set_var_from_var(const Numer
static char *get_str_from_var(const NumericVar *var);
static char *get_str_from_var_sci(const NumericVar *var, int rscale);
+static void numericvar_serialize(StringInfo buf, const NumericVar *var);
+static void numericvar_deserialize(StringInfo buf, NumericVar *var);
+
static Numeric duplicate_numeric(Numeric num);
static Numeric make_result(const NumericVar *var);
static Numeric make_result_opt_error(const NumericVar *var, bool *error);
@@ -4943,38 +4946,25 @@ numeric_avg_serialize(PG_FUNCTION_ARGS)
{
NumericAggState *state;
StringInfoData buf;
- Datum temp;
- bytea *sumX;
bytea *result;
NumericVar tmp_var;
+ init_var(&tmp_var);
+
/* Ensure we disallow calling when not in aggregate context */
if (!AggCheckCallContext(fcinfo, NULL))
elog(ERROR, "aggregate function called in non-aggregate context");
state = (NumericAggState *) PG_GETARG_POINTER(0);
- /*
- * This is a little wasteful since make_result converts the NumericVar
- * into a Numeric and numeric_send converts it back again. Is it worth
- * splitting the tasks in numeric_send into separate functions to stop
- * this? Doing so would also remove the fmgr call overhead.
- */
- init_var(&tmp_var);
- accum_sum_final(&state->sumX, &tmp_var);
-
- temp = DirectFunctionCall1(numeric_send,
- NumericGetDatum(make_result(&tmp_var)));
- sumX = DatumGetByteaPP(temp);
- free_var(&tmp_var);
-
pq_begintypsend(&buf);
/* N */
pq_sendint64(&buf, state->N);
/* sumX */
- pq_sendbytes(&buf, VARDATA_ANY(sumX), VARSIZE_ANY_EXHDR(sumX));
+ accum_sum_final(&state->sumX, &tmp_var);
+ numericvar_serialize(&buf, &tmp_var);
/* maxScale */
pq_sendint32(&buf, state->maxScale);
@@ -4993,6 +4983,8 @@ numeric_avg_serialize(PG_FUNCTION_ARGS)
result = pq_endtypsend(&buf);
+ free_var(&tmp_var);
+
PG_RETURN_BYTEA_P(result);
}
@@ -5006,9 +4998,10 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS
{
bytea *sstate;
NumericAggState *result;
- Datum temp;
- NumericVar tmp_var;
StringInfoData buf;
+ NumericVar tmp_var;
+
+ init_var(&tmp_var);
if (!AggCheckCallContext(fcinfo, NULL))
elog(ERROR, "aggregate function called in non-aggregate context");
@@ -5029,11 +5022,7 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS
result->N = pq_getmsgint64(&buf);
/* sumX */
- temp = DirectFunctionCall3(numeric_recv,
- PointerGetDatum(&buf),
- ObjectIdGetDatum(InvalidOid),
- Int32GetDatum(-1));
- init_var_from_num(DatumGetNumeric(temp), &tmp_var);
+ numericvar_deserialize(&buf, &tmp_var);
accum_sum_add(&(result->sumX), &tmp_var);
/* maxScale */
@@ -5054,6 +5043,8 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS
pq_getmsgend(&buf);
pfree(buf.data);
+ free_var(&tmp_var);
+
PG_RETURN_POINTER(result);
}
@@ -5067,11 +5058,10 @@ numeric_serialize(PG_FUNCTION_ARGS)
{
NumericAggState *state;
StringInfoData buf;
- Datum temp;
- bytea *sumX;
- NumericVar tmp_var;
- bytea *sumX2;
bytea *result;
+ NumericVar tmp_var;
+
+ init_var(&tmp_var);
/* Ensure we disallow calling when not in aggregate context */
if (!AggCheckCallContext(fcinfo, NULL))
@@ -5079,36 +5069,18 @@ numeric_serialize(PG_FUNCTION_ARGS)
state = (NumericAggState *) PG_GETARG_POINTER(0);
- /*
- * This is a little wasteful since make_result converts the NumericVar
- * into a Numeric and numeric_send converts it back again. Is it worth
- * splitting the tasks in numeric_send into separate functions to stop
- * this? Doing so would also remove the fmgr call overhead.
- */
- init_var(&tmp_var);
-
- accum_sum_final(&state->sumX, &tmp_v
Re: Signed vs. Unsigned (some)
Em sex., 2 de jul. de 2021 às 07:09, Peter Eisentraut < peter.eisentr...@enterprisedb.com> escreveu: > On 16.06.21 10:48, Peter Eisentraut wrote: > > On 15.06.21 10:17, Kyotaro Horiguchi wrote: > >> The definitions are not ((type) -1) but ((type) 0x) so > >> actually they might be different if we forget to widen the constant > >> when widening the types. Regarding to the compiler behavior, I think > >> we are assuming C99[1] and C99 defines that -1 is converted to > >> Uxxx_MAX. (6.3.1.3 Singed and unsigned integers) > >> > >> I'm +0.2 on it. It might be worthwhile as a matter of style. > > > > I think since we have the constants we should use them. > > I have pushed the InvalidBucket changes. > Nice. Thanks. > The use of InvalidBlockNumber with vac_update_relstats() looks a bit > fishy to me. We are using in the same call 0 as the default for > num_all_visible_pages, and we generally elsewhere also use 0 as the > starting value for relpages, so it's not clear to me why it should be -1 > or InvalidBlockNumber here. It seems to me that the only use in vac_update_relstats is to mark relpages as invalid (dirty = true). > I'd rather leave it "slightly wrong" for > now so it can be checked again. > Ideally InvalidBlockNumber should be 0. Maybe in the long run this will be fixed. regards, Ranier Vilela
Re: Schema variables - new implementation for Postgres 15
so 12. 6. 2021 v 8:00 odesílatel Pavel Stehule napsal: > Hi > > rebase > > rebase only Regards Pavel Regards > > Pavel > schema-variables-20210702.patch.gz Description: application/gzip
Re: Update maintenance_work_mem/autovacuum_work_mem to reflect the 1GB limitation with VACUUM
On Fri, 21 May 2021 at 03:52, Laurenz Albe wrote: > Just sending a reply to -hackers so I can add it to the commitfest. I had a look at the patch in [1] and I find it a bit weird that we'd write the following about autovacuum_work_mem in our docs: + +Note that VACUUM has a hard-coded limit of 1GB +for the amount of memory used, so setting +autovacuum_work_mem higher than that has no effect. + Since that setting is *only* used for auto vacuum, why don't we just limit the range of the GUC to 1GB? Of course, it wouldn't be wise to backpatch the reduced limit of autovacuum_work_mem as it might upset people who have higher values in their postgresql.conf when their database fails to restart after an upgrade. I think what might be best is just to reduce the limit in master and apply the doc patch for just maintenance_work_mem in all supported versions. We could just ignore doing anything with autovacuum_work_mem in the back branches and put it down to a historical mistake that can't easily be fixed now. I've attached what I came up with. What do you think? [1] https://www.postgresql.org/message-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel%40cybertec.at limit_vacuum_memory_to_1gb.patch Description: Binary data
Re: Numeric multiplication overflow errors
On Fri, 2 Jul 2021 at 22:55, Dean Rasheed wrote: > Here's an update with the > last set of changes discussed. Looks good to me. Just the question of if we have any problems changing the serialized format in the back branches. I'm not sure if that's something we've done before. I only had a quick look of git blame in the serial/deserial functions and the only changes I really see apart from a few cosmetic ones were a57d312a7 and 9cca11c91. Both of which just went into master. David
Re: Commit fest manager
On Fri, 2 Jul 2021 at 1:47 PM, David Rowley wrote: > On Fri, 2 Jul 2021 at 15:04, vignesh C wrote: > > I'm interested in assisting Ibrar Ahmed. > > It might be worth talking to Ibrar to see where you can lend a hand. I > think in terms of the number of patches, this might be our biggest > commitfest yet. > > 2020-07 246 > 2020-09 235 > 2020-11 244 > 2021-01 260 > 2020-03 295 > 2020-07 342 > > It's possible Ibrar would welcome you helping to take care of some of > the duties. I've never been brave enough to take on the CF manager > role yet, but from what I can see, to do a good job takes a huge > amount of effort. > > David I am willing to take the responsibility, help from vegnsh is welcome > > -- Ibrar Ahmed
Re: Update maintenance_work_mem/autovacuum_work_mem to reflect the 1GB limitation with VACUUM
On Fri, 2021-07-02 at 23:31 +1200, David Rowley wrote: > I had a look at the patch in [1] and I find it a bit weird that we'd > write the following about autovacuum_work_mem in our docs: > > + > +Note that VACUUM has a hard-coded limit of 1GB > +for the amount of memory used, so setting > +autovacuum_work_mem higher than that has no > effect. > + > > Since that setting is *only* used for auto vacuum, why don't we just > limit the range of the GUC to 1GB? > > Of course, it wouldn't be wise to backpatch the reduced limit of > autovacuum_work_mem as it might upset people who have higher values in > their postgresql.conf when their database fails to restart after an > upgrade. I think what might be best is just to reduce the limit in > master and apply the doc patch for just maintenance_work_mem in all > supported versions. We could just ignore doing anything with > autovacuum_work_mem in the back branches and put it down to a > historical mistake that can't easily be fixed now. > > I've attached what I came up with. > > What do you think? > > [1] > https://www.postgresql.org/message-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel%40cybertec.at I think that is much better. I am fine with that patch. Yours, Laurenz Albe
Re: Replication protocol doc fix
On Fri, Jul 2, 2021 at 1:55 AM Jeff Davis wrote: > On Wed, 2021-06-30 at 12:25 -0400, Robert Haas wrote: > > I am not sure whether this works or not. Holding off cancel > > interrupts > > across possible network I/O seems like a non-starter. We have to be > > able to kill off connections that have wedged. > > I was following a pattern that I saw in CopyGetData() and > SocketBackend(). If I understand correctly, the idea is to avoid a > cancel leaving part of a message unread, which would desync the > protocol. Right, that seems like a good goal. Thinking about this a little more, it's only holding off *cancel* interrupts, not *all* interrupts, so presumably you can still terminate the backend in this state. That's not so bad, and it's not clear how we could do any better. So I withdraw my previous complaint about this point. -- Robert Haas EDB: http://www.enterprisedb.com
RE: ECPG bug fix: DECALRE STATEMENT and DEALLOCATE, DESCRIBE
Dear Hackers, I revised my patch. > Please also ensure that you're generating the patch against git HEAD. > The cfbot shows it as failing to apply, likely because you're looking > at something predating ad8305a43d1890768a613d3fb586b44f17360f29. Maybe there was something wrong with my local environment. Sorry. > However, I perfectly agree that it's very difficult for users to find a > problem from the message. > I will try to add information to it in the next patch. I added such a message and some tests, but I began to think this is strange. Now I'm wondering why the connection is checked in some DESCRIPTOR-related statements? In my understanding connection name is not used in ECPGallocate_desc(), ECPGdeallocate_desc(), ECPGget_desc() and so on. Hence I think lookup_descriptor() and drop_descriptor() can be removed. This idea can solve your first argument. > You're right. This is very stupid program. I'll combine them into one. Check_declared_list() was moved to stmt:ECPGDescribe rule. Some similar rules still remain in ecpg.trailer, but INPUT/OUTPUT statements have different rules and actions and I cannot combine well. Best Regards, Hayato Kuroda FUJITSU LIMITED v02-0001-fix-deallocate-describe.patch Description: v02-0001-fix-deallocate-describe.patch v02-0002-add-test.patch Description: v02-0002-add-test.patch
Re: Mention --enable-tap-tests in the TAP section page
On 7/1/21 9:53 PM, Michael Paquier wrote: > On Thu, Jul 01, 2021 at 10:03:10AM -0400, Greg Sabino Mullane wrote: >> Searching about the TAP tests often leads to this page, but there is no >> easy link or mention of the fact that the sample invocations will not work >> without the special config flag. > This is mentioned on a different page, "Running the Tests", but for > the set of extra tests. Adding an extra reference on this page is a > good idea. Agreed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Commit fest manager
On Fri, Jul 2, 2021 at 6:05 PM Ibrar Ahmed wrote: > > > > On Fri, 2 Jul 2021 at 1:47 PM, David Rowley wrote: >> >> On Fri, 2 Jul 2021 at 15:04, vignesh C wrote: >> > I'm interested in assisting Ibrar Ahmed. >> >> It might be worth talking to Ibrar to see where you can lend a hand. I >> think in terms of the number of patches, this might be our biggest >> commitfest yet. >> >> 2020-07 246 >> 2020-09 235 >> 2020-11 244 >> 2021-01 260 >> 2020-03 295 >> 2020-07 342 >> >> It's possible Ibrar would welcome you helping to take care of some of >> the duties. I've never been brave enough to take on the CF manager >> role yet, but from what I can see, to do a good job takes a huge >> amount of effort. >> >> David > > > I am willing to take the responsibility, help from vegnsh is welcome Thanks, Can someone provide me permissions as this will be my first time. My username is vignesh.postgres. Regards, Vignesh
Re: Commit fest manager
On Fri, 2 Jul 2021 at 7:06 PM, vignesh C wrote: > On Fri, Jul 2, 2021 at 6:05 PM Ibrar Ahmed wrote: > > > > > > > > On Fri, 2 Jul 2021 at 1:47 PM, David Rowley > wrote: > >> > >> On Fri, 2 Jul 2021 at 15:04, vignesh C wrote: > >> > I'm interested in assisting Ibrar Ahmed. > >> > >> It might be worth talking to Ibrar to see where you can lend a hand. I > >> think in terms of the number of patches, this might be our biggest > >> commitfest yet. > >> > >> 2020-07 246 > >> 2020-09 235 > >> 2020-11 244 > >> 2021-01 260 > >> 2020-03 295 > >> 2020-07 342 > >> > >> It's possible Ibrar would welcome you helping to take care of some of > >> the duties. I've never been brave enough to take on the CF manager > >> role yet, but from what I can see, to do a good job takes a huge > >> amount of effort. > >> > >> David > > > > > > I am willing to take the responsibility, help from vegnsh is welcome > > Thanks, Can someone provide me permissions as this will be my first > time. My username is vignesh.postgres. > > Regards, > Vignesh i need permission my id is ibrar.ah...@gmail.com > > -- Ibrar Ahmed
Re: RFC: Logging plan of the running query
On Tue, Jun 22, 2021 at 8:00 AM torikoshia wrote:
> Updated the patch.
Thanks for the patch. Here are some comments on the v4 patch:
1) Can we do + ExplainState *es = NewExplainState(); and es
assignments after if (!ActivePortal || !ActivePortal->queryDesc), just
to avoid unnecessary call in case of error hit? Also note that, we can
easily hit the error case.
2) It looks like there's an improper indentation. MyProcPid and
es->str->data, should start from the ".
+ ereport(LOG_SERVER_ONLY,
+ (errmsg("backend with PID %d is not running a query",
+ MyProcPid)));
+ ereport(LOG_SERVER_ONLY,
+ (errmsg("plan of the query running on backend with PID %d is:\n%s",
+ MyProcPid,
+ es->str->data),
For reference see errmsg("unrecognized value for EXPLAIN option \"%s\": \"%s\"",
3)I prefer to do this so that any new piece of code can be introduced
in between easily and it will be more readable as well.
+Datum
+pg_log_current_query_plan(PG_FUNCTION_ARGS)
+{
+ pid_t pid;
+ bool result;
+
+ pid = PG_GETARG_INT32(0);
+ result = SendProcSignalForLogInfo(pid, PROCSIG_LOG_CURRENT_PLAN);
+
+ PG_RETURN_BOOL(result);
+}
If okay, please also change for the pg_log_backend_memory_contexts.
4) Extra whitespace before the second line i.e. 2nd line reason should
be aligned with the 1st line reason.
+ Assert(reason == PROCSIG_LOG_MEMORY_CONTEXT ||
+ reason == PROCSIG_LOG_CURRENT_PLAN);
5) How about "Requests to log the plan of the query currently running
on the backend with specified process ID along with the untruncated
query string"?
+Requests to log the untruncated query string and its plan for
+the query currently running on the backend with the specified
+process ID.
6) A typo: it is "nested statements (..) are not"
+Note that nested statements (statements executed inside a function) is not
7) I'm not sure what you mean by "Some functions output what you want
to the log."
--- Memory contexts are logged and they are not returned to the function.
+-- Some functions output what you want to the log.
Instead, can we say "These functions return true if the specified
backend is successfully signaled, otherwise false. Upon receiving the
signal, the backend will log the information to the server log."
Regards,
Bharath Rupireddy.
Re: [bug?] Missed parallel safety checks, and wrong parallel safety
On Wed, Jun 30, 2021 at 11:46 PM Greg Nancarrow wrote: > I personally think "(b) provide an option to the user to specify > whether inserts can be parallelized on a relation" is the preferable > option. > There seems to be too many issues with the alternative of trying to > determine the parallel-safety of a partitioned table automatically. > I think (b) is the simplest and most consistent approach, working the > same way for all table types, and without the overhead of (a). > Also, I don't think (b) is difficult for the user. At worst, the user > can use the provided utility-functions at development-time to verify > the intended declared table parallel-safety. > I can't really see some mixture of (a) and (b) being acceptable. Yeah, I'd like to have it be automatic, but I don't have a clear idea how to make that work nicely. It's possible somebody (Tom?) can suggest something that I'm overlooking, though. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Signed vs. Unsigned (some)
On Fri, Jul 02, 2021 at 12:09:23PM +0200, Peter Eisentraut wrote: > On 16.06.21 10:48, Peter Eisentraut wrote: > > On 15.06.21 10:17, Kyotaro Horiguchi wrote: > > > The definitions are not ((type) -1) but ((type) 0x) so > > > actually they might be different if we forget to widen the constant > > > when widening the types. Regarding to the compiler behavior, I think > > > we are assuming C99[1] and C99 defines that -1 is converted to > > > Uxxx_MAX. (6.3.1.3 Singed and unsigned integers) > > > > > > I'm +0.2 on it. It might be worthwhile as a matter of style. > > > > I think since we have the constants we should use them. > > I have pushed the InvalidBucket changes. > > The use of InvalidBlockNumber with vac_update_relstats() looks a bit fishy > to me. We are using in the same call 0 as the default for > num_all_visible_pages, and we generally elsewhere also use 0 as the starting > value for relpages, so it's not clear to me why it should be -1 or > InvalidBlockNumber here. I'd rather leave it "slightly wrong" for now so it > can be checked again. There's two relevant changes: |commit 3d351d916b20534f973eda760cde17d96545d4c4 |Author: Tom Lane |Date: Sun Aug 30 12:21:51 2020 -0400 | |Redefine pg_class.reltuples to be -1 before the first VACUUM or ANALYZE. |commit 0e69f705cc1a3df273b38c9883fb5765991e04fe |Author: Alvaro Herrera |Date: Fri Apr 9 11:29:08 2021 -0400 | |Set pg_class.reltuples for partitioned tables 3d35 also affects partitioned tables, and 0e69 appears to do the right thing by preserving relpages=-1 during auto-analyze. Note that Alvaro's commit message and comment refer to relpages, but should have said reltuples - comment fixed at 7ef8b52cf. -- Justin
Re: Detecting File Damage & Inconsistencies
On Fri, Jul 2, 2021 at 5:34 AM Craig Ringer wrote: > > On Fri, 2 Jul 2021 at 00:19, Simon Riggs wrote: > >> >> > So yeah. I think it'd be better to log the info you want at start-of-txn >> > unless there's a compelling reason not so, and I don't see one yet. >> >> AFAIK, XLOG_XACT_ASSIGNMENT does not occur for normal top-level >> transactions, only for subxids. >> >> I don't really want to add an extra record just for this because it >> will slow down applications and it won't get turned on as often. > > > OK, that makes sense - I was indeed operating on an incorrect assumption. > > I wouldn't want to add a new record either. I thought we could piggyback on > XLOG_XACT_ASSIGNMENT with a new chunk that's only added when the feature is > required, much like we do for replication origin info on commit records. > > Is it worth considering forcing XLOG_XACT_ASSIGNMENT to be logged if this > feature is enabled? My feeling is that the drop in performance would lead to it being turned off most of the time, reducing the value of the feature. Does anyone else disagree? > If you don't think the sorts of use cases I presented are worth the trouble > that's fair enough. I'm not against adding it on the commit record. It's just > that with logical decoding moving toward a streaming model I suspect only > having it at commit time may cause us some pain later. I think you have some good ideas about how to handle larger transactions with streaming. As a separate patch it might be worth keeping track of transaction size and logging something when a transaction gets too large. -- Simon Riggshttp://www.EnterpriseDB.com/
Re: Preventing abort() and exit() calls in libpq
Now it's hoverfly: ! nm -A -u libpq.so.5 2>/dev/null | grep -v __cxa_atexit | grep exit libpq.so.5: atexit U - libpq.so.5: pthread_exit U - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2021-07-02%2010%3A10%3A29 -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Use it up, wear it out, make it do, or do without"
Re: psql - factor out echo code
Fabien COELHO writes:
> [ psql-echo-2.patch ]
I went to commit this, figuring that it was a trivial bit of code
consolidation, but as I looked around in common.c I got rather
unhappy with the inconsistent behavior of things. Examining
the various places that implement "echo"-related logic, we have
the three places this patch proposes to unify, which log queries
using
fprintf(out,
_("* QUERY **\n"
"%s\n"
"**\n\n"), query);
and then we have two more that just do
puts(query);
plus this:
if (!OK && pset.echo == PSQL_ECHO_ERRORS)
pg_log_info("STATEMENT: %s", query);
So it's exactly fifty-fifty as to whether we add all that decoration
or none at all. I think if we're going to touch this logic, we
ought to try to unify the behavior. My vote would be to drop the
decoration everywhere, but perhaps there are votes not to?
A different angle is that the identical decoration is used for both
psql-generated queries that are logged because of ECHO_HIDDEN, and
user-entered queries. This seems at best rather unhelpful. If
we keep the decoration, should we make it different for those two
cases? (Maybe "INTERNAL QUERY" vs "QUERY", for example.) The
cases with no decoration likewise fall into multiple categories,
both user-entered and generated-by-gexec; if we were going with
a decorated approach I'd think it useful to make a distinction
between those, too.
Thoughts?
regards, tom lane
Re: Preventing abort() and exit() calls in libpq
Alvaro Herrera writes: > Now it's hoverfly: > ! nm -A -u libpq.so.5 2>/dev/null | grep -v __cxa_atexit | grep exit > libpq.so.5: atexit U - > libpq.so.5: pthread_exit U - Ugh. What in the world is producing those references? (As I mentioned upthread, I'm quite suspicious of libpq trying to perform any actions in an atexit callback, because of the uncertainty about whether some later atexit callback could try to use libpq functions. So this seems like it might be an actual bug.) regards, tom lane
Re: wrong relkind error messages
On 2021-Jun-24, Peter Eisentraut wrote: > There might be room for some wordsmithing in a few places, but generally I > think this is complete. This looks good to me. I am +0.1 on your proposal of "cannot have triggers" vs Michael's "cannot create triggers", but really I could go with either. Michael's idea has the disadvantage that if the user fails to see the trailing "s" in "triggers" they could get the idea that it's possible to create some other trigger; that seems impossible to miss with your wording. But it's not that bad either. It seemed odd to me at first that errdetail_relkind_not_supported() returns int, but I realized that it's a trick to let you write "return errdetail()" so you don't have to have "break" which would require one extra line. Looks fine. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Signed vs. Unsigned (some)
On 2021-Jul-02, Justin Pryzby wrote: > On Fri, Jul 02, 2021 at 12:09:23PM +0200, Peter Eisentraut wrote: > > The use of InvalidBlockNumber with vac_update_relstats() looks a bit fishy > > to me. We are using in the same call 0 as the default for > > num_all_visible_pages, and we generally elsewhere also use 0 as the starting > > value for relpages, so it's not clear to me why it should be -1 or > > InvalidBlockNumber here. I'd rather leave it "slightly wrong" for now so it > > can be checked again. > |commit 0e69f705cc1a3df273b38c9883fb5765991e04fe > |Author: Alvaro Herrera > |Date: Fri Apr 9 11:29:08 2021 -0400 > | > |Set pg_class.reltuples for partitioned tables > > 3d35 also affects partitioned tables, and 0e69 appears to do the right thing > by > preserving relpages=-1 during auto-analyze. I suppose the question is what is the value used for. BlockNumber is typedef'd uint32, an unsigned variable, so using -1 for it is quite fishy. The weird thing is that in vac_update_relstats we cast it to (int32) when storing it in the pg_class tuple, so that's quite fishy too. What we really want is for table_block_relation_estimate_size to work properly. What that does is get the signed-int32 value from pg_class and cast it back to BlockNumber. If that assignment gets -1 again, then it's all fine. I didn't test it. I think changing the vac_update_relstats call I added in 0e69f705cc1a to InvalidBlockNumber is fine. I didn't verify any other places. I think storing BlockNumber values >= 2^31 in an int32 catalog column is asking for trouble. We'll have to fix that at some point. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
Back-branch commit complexity
In my git workflow, I normally use scripts to simplify and check things. Previously, most of my workfload was on master, with patches migrated to appropriate back branches. FYI, now that we have the release notes only in the major version branches, I have had to adjust my scripts to allow for more per-major-version branches and automated doc builds of back branches. I thought people might like to know if they come upon the same issue. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Signed vs. Unsigned (some)
Em sex., 2 de jul. de 2021 às 13:29, Alvaro Herrera
escreveu:
> On 2021-Jul-02, Justin Pryzby wrote:
>
> > On Fri, Jul 02, 2021 at 12:09:23PM +0200, Peter Eisentraut wrote:
>
> > > The use of InvalidBlockNumber with vac_update_relstats() looks a bit
> fishy
> > > to me. We are using in the same call 0 as the default for
> > > num_all_visible_pages, and we generally elsewhere also use 0 as the
> starting
> > > value for relpages, so it's not clear to me why it should be -1 or
> > > InvalidBlockNumber here. I'd rather leave it "slightly wrong" for now
> so it
> > > can be checked again.
>
> > |commit 0e69f705cc1a3df273b38c9883fb5765991e04fe
> > |Author: Alvaro Herrera
> > |Date: Fri Apr 9 11:29:08 2021 -0400
> > |
> > |Set pg_class.reltuples for partitioned tables
> >
> > 3d35 also affects partitioned tables, and 0e69 appears to do the right
> thing by
> > preserving relpages=-1 during auto-analyze.
>
> I suppose the question is what is the value used for. BlockNumber is
> typedef'd uint32, an unsigned variable, so using -1 for it is quite
> fishy. The weird thing is that in vac_update_relstats we cast it to
> (int32) when storing it in the pg_class tuple, so that's quite fishy
> too.
>
> What we really want is for table_block_relation_estimate_size to work
> properly. What that does is get the signed-int32 value from pg_class
> and cast it back to BlockNumber. If that assignment gets -1 again, then
> it's all fine. I didn't test it.
>
It seems to me that it is happening, but it is risky to make comparisons
between different types.
1)
#define InvalidBlockNumber 0x
int main()
{
unsigned int num_pages;
int rel_pages;
num_pages = -1;
rel_pages = (int) num_pages;
printf("num_pages = %u\n", num_pages);
printf("rel_pages = %d\n", rel_pages);
printf("(num_pages == InvalidBlockNumber) => %u\n", (num_pages ==
InvalidBlockNumber));
printf("(rel_pages == InvalidBlockNumber) => %u\n", (rel_pages ==
InvalidBlockNumber));
}
num_pages = 4294967295
rel_pages = -1
(num_pages == InvalidBlockNumber) => 1
(rel_pages == InvalidBlockNumber) => 1 /* 17:68: warning: comparison
between signed and unsigned integer expressions [-Wsign-compare] */
If num_pages is promoted to uint64 and rel_pages to int64:
2)
#define InvalidBlockNumber 0x
int main()
{
unsigned long int num_pages;
long int rel_pages;
num_pages = -1;
rel_pages = (int) num_pages;
printf("num_pages = %lu\n", num_pages);
printf("rel_pages = %ld\n", rel_pages);
printf("(num_pages == InvalidBlockNumber) => %u\n", (num_pages ==
InvalidBlockNumber));
printf("(rel_pages == InvalidBlockNumber) => %u\n", (rel_pages ==
InvalidBlockNumber));
}
num_pages = 18446744073709551615
rel_pages = -1
(num_pages == InvalidBlockNumber) => 0
(rel_pages == InvalidBlockNumber) => 0 /* 17:68: warning: comparison
between signed and unsigned integer expressions [-Wsign-compare] */
As Kyotaro said:
"they might be different if we forget to widen the constant
when widening the types"
regards,
Ranier Vilela
Re: Increase value of OUTER_VAR
Here's a more fleshed-out version of this patch. I ran around and
fixed all the places where INNER_VAR etc. were being assigned directly to
a variable or parameter of type Index, and also grepped for 'Index.*varno'
to find suspicious declarations. (I didn't change every last instance
of the latter though; just places that could possibly be looking at
post-setrefs.c Vars.)
I concluded that we don't really need to change the type of
CurrentOfExpr.cvarno, because that's never set to a special value.
The main thing I remain concerned about is whether there are more
places like set_pathtarget_cost_width(), where we could be making
an inequality comparison on "varno" that would now be wrong.
I tried to catch this by enabling -Wsign-compare and -Wsign-conversion,
but that produced so many thousands of uninteresting warnings that
I soon gave up. I'm not sure there's any good way to catch remaining
places like that except to commit the patch and wait for trouble
reports.
So I'm inclined to propose pushing this and seeing what happens.
regards, tom lane
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index 69ab34573e..9f1d8b6d1e 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -282,7 +282,7 @@ ExecAssignScanProjectionInfo(ScanState *node)
* As above, but caller can specify varno expected in Vars in the tlist.
*/
void
-ExecAssignScanProjectionInfoWithVarno(ScanState *node, Index varno)
+ExecAssignScanProjectionInfoWithVarno(ScanState *node, int varno)
{
TupleDesc tupdesc = node->ss_ScanTupleSlot->tts_tupleDescriptor;
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index ad11392b99..4ab1302313 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -65,7 +65,7 @@
#include "utils/typcache.h"
-static bool tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc);
+static bool tlist_matches_tupdesc(PlanState *ps, List *tlist, int varno, TupleDesc tupdesc);
static void ShutdownExprContext(ExprContext *econtext, bool isCommit);
@@ -553,7 +553,7 @@ ExecAssignProjectionInfo(PlanState *planstate,
*/
void
ExecConditionalAssignProjectionInfo(PlanState *planstate, TupleDesc inputDesc,
- Index varno)
+ int varno)
{
if (tlist_matches_tupdesc(planstate,
planstate->plan->targetlist,
@@ -579,7 +579,7 @@ ExecConditionalAssignProjectionInfo(PlanState *planstate, TupleDesc inputDesc,
}
static bool
-tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc)
+tlist_matches_tupdesc(PlanState *ps, List *tlist, int varno, TupleDesc tupdesc)
{
int numattrs = tupdesc->natts;
int attrno;
diff --git a/src/backend/executor/nodeCustom.c b/src/backend/executor/nodeCustom.c
index c82060e6d1..1dfa53c381 100644
--- a/src/backend/executor/nodeCustom.c
+++ b/src/backend/executor/nodeCustom.c
@@ -31,7 +31,7 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
CustomScanState *css;
Relation scan_rel = NULL;
Index scanrelid = cscan->scan.scanrelid;
- Index tlistvarno;
+ int tlistvarno;
/*
* Allocate the CustomScanState object. We let the custom scan provider
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9dc38d47ea..cd93d1d768 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -128,7 +128,7 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
ForeignScanState *scanstate;
Relation currentRelation = NULL;
Index scanrelid = node->scan.scanrelid;
- Index tlistvarno;
+ int tlistvarno;
FdwRoutine *fdwroutine;
/* check for unsupported flags */
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 01c110cd2f..7d1a01d1ed 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -63,7 +63,7 @@ makeSimpleA_Expr(A_Expr_Kind kind, char *name,
* creates a Var node
*/
Var *
-makeVar(Index varno,
+makeVar(int varno,
AttrNumber varattno,
Oid vartype,
int32 vartypmod,
@@ -85,7 +85,7 @@ makeVar(Index varno,
* them, but just initialize them to the given varno/varattno. This
* reduces code clutter and chance of error for most callers.
*/
- var->varnosyn = varno;
+ var->varnosyn = (Index) varno;
var->varattnosyn = varattno;
/* Likewise, we just set location to "unknown" here */
@@ -100,7 +100,7 @@ makeVar(Index varno,
* TargetEntry
*/
Var *
-makeVarFromTargetEntry(Index varno,
+makeVarFromTargetEntry(int varno,
TargetEntry *tle)
{
return makeVar(varno,
@@ -131,7 +131,7 @@ makeVarFromTargetEntry(Index varno,
*/
Var *
makeWholeRowVar(RangeTblEntry *rte,
-Index varno,
+int varno,
Index varlevelsup,
bool allowScalar)
{
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index e32b92e299..bcb116ae8d 10
Re: Numeric multiplication overflow errors
On Fri, 2 Jul 2021 at 22:55, Dean Rasheed wrote: > Here's an update with the > last set of changes discussed. If you allow me a small suggestion. Move the initializations of the variable tmp_var to after check if the function can run. Saves some cycles, when not running. /* Ensure we disallow calling when not in aggregate context */ if (!AggCheckCallContext(fcinfo, NULL)) elog(ERROR, "aggregate function called in non-aggregate context"); + init_var(&tmp_var); + regards, Ranier Vilela
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
On Tue, Apr 06, 2021 at 11:01:31AM -0500, Justin Pryzby wrote:
> On Wed, Dec 23, 2020 at 01:17:10PM -0600, Justin Pryzby wrote:
> > On Mon, Nov 23, 2020 at 04:14:18PM -0500, Tom Lane wrote:
> > > * I noticed that you did s/stat/lstat/. That's fine on Unix systems,
> > > but it won't have any effect on Windows systems (cf bed90759f),
> > > which means that we'll have to document a platform-specific behavioral
> > > difference. Do we want to go there?
> > >
> > > Maybe this patch needs to wait on somebody fixing our lack of real
> > > lstat() on Windows.
> >
> > I think only the "top" patches depend on lstat (for the "type" column and
> > recursion, to avoid loops). The initial patches are independently useful,
> > and
> > resolve the original issue of hiding tmpdirs. I've rebased and re-arranged
> > the
> > patches to reflect this.
>
> I said that, but then failed to attach the re-arranged patches.
> Now I also renumbered OIDs following best practice.
>
> The first handful of patches address the original issue, and I think could be
> "ready":
>
> $ git log --oneline origin..pg-ls-dir-new |tac
> ... Document historic behavior of links to directories..
> ... Add tests on pg_ls_dir before changing it
> ... Add pg_ls_dir_metadata to list a dir with file metadata..
> ... pg_ls_tmpdir to show directories and "isdir" argument..
> ... pg_ls_*dir to show directories and "isdir" column..
>
> These others are optional:
> ... pg_ls_logdir to ignore error if initial/top dir is missing..
> ... pg_ls_*dir to return all the metadata from pg_stat_file..
>
> ..and these maybe requires more work for lstat on windows:
> ... pg_stat_file and pg_ls_dir_* to use lstat()..
> ... pg_ls_*/pg_stat_file to show file *type*..
> ... Preserve pg_stat_file() isdir..
> ... Add recursion option in pg_ls_dir_files..
@cfbot: rebased
>From 4b34d394af0253dc3be2e47cb2e8ccd286eea0a3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Mon, 16 Mar 2020 14:12:55 -0500
Subject: [PATCH v30 01/11] Document historic behavior of links to
directories..
Backpatch to 9.5: pg_stat_file
---
doc/src/sgml/func.sgml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6388385edc..7a830f0684 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27004,7 +27004,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
Returns a record containing the file's size, last access time stamp,
last modification time stamp, last file status change time stamp (Unix
platforms only), file creation time stamp (Windows only), and a flag
-indicating if it is a directory.
+indicating if it is a directory (or a symbolic link to a directory).
This function is restricted to superusers by default, but other users
--
2.17.0
>From effa738721a008a5d0c65defecfb7b36ab8f33b3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 17 Mar 2020 13:16:24 -0500
Subject: [PATCH v30 02/11] Add tests on pg_ls_dir before changing it
---
src/test/regress/expected/misc_functions.out | 24
src/test/regress/sql/misc_functions.sql | 8 +++
2 files changed, 32 insertions(+)
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38..ea0fc48dbd 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -214,6 +214,30 @@ select count(*) > 0 from
t
(1 row)
+select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true
+ name
+--
+ .
+(1 row)
+
+select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false
+ name
+--
+(0 rows)
+
+select pg_ls_dir('does not exist', true, false); -- ok with missingok=true
+ pg_ls_dir
+---
+(0 rows)
+
+select pg_ls_dir('does not exist'); -- fails with missingok=false
+ERROR: could not open directory "does not exist": No such file or directory
+-- Check that expected columns are present
+select * from pg_stat_file('.') limit 0;
+ size | access | modification | change | creation | isdir
+--++--++--+---
+(0 rows)
+
--
-- Test adding a support function to a subject function
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc..eb6ac12ab4 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -69,6 +69,14 @@ select count(*) > 0 from
where spcname = 'pg_default') pts
join pg_database db on pts.pts = db.oid;
+select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true
+select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false
+select pg_ls_dir('does not exist', true, fa
Re: psql - factor out echo code
Hello Tom,
I went to commit this, figuring that it was a trivial bit of code
consolidation, but as I looked around in common.c I got rather
unhappy with the inconsistent behavior of things. Examining
the various places that implement "echo"-related logic, we have
the three places this patch proposes to unify, which log queries
using
fprintf(out,
_("* QUERY **\n"
"%s\n"
"**\n\n"), query);
and then we have two more that just do
puts(query);
plus this:
if (!OK && pset.echo == PSQL_ECHO_ERRORS)
pg_log_info("STATEMENT: %s", query);
So it's exactly fifty-fifty as to whether we add all that decoration
or none at all. I think if we're going to touch this logic, we
ought to try to unify the behavior.
+1.
I did not go this way because I wanted it to be a simple restructuring
patch so that it could go through without much ado, but I agree with
improving the current status. I'm not sure we want too much ascii-art.
My vote would be to drop the decoration everywhere, but perhaps there
are votes not to?
No, I'd be ok with removing the decoration, or at least simplify them, or
as you suggest below make the have a useful semantics.
A different angle is that the identical decoration is used for both
psql-generated queries that are logged because of ECHO_HIDDEN, and
user-entered queries. This seems at best rather unhelpful.
Indeed.
If we keep the decoration, should we make it different for those two
cases? (Maybe "INTERNAL QUERY" vs "QUERY", for example.) The cases
with no decoration likewise fall into multiple categories, both
user-entered and generated-by-gexec; if we were going with a decorated
approach I'd think it useful to make a distinction between those, too.
Thoughts?
Yes. Maybe decorations should be SQL comments, and the purpose/origin of
the query could be made clear as you suggest, eg something like markdown
in a comment:
"-- # QUERY\n%s\n\n"
with in USER DESCRIPTION COMPLETION GEXEC…
--
Fabien.
Re: logical replication worker accesses catalogs in error context callback
Bharath Rupireddy writes: >> << Attaching v5-0001 here again for completion >> >> I'm attaching v5-0001 patch that avoids printing the column type names >> in the context message thus no cache lookups have to be done in the >> error context callback. I think the column name is enough to know on >> which column the error occurred and if required it's type can be known >> by the user. This patch gets rid of printing local and remote type >> names in slot_store_error_callback and also >> logicalrep_typmap_gettypname because it's unnecessary. Thoughts? I've now pushed this patch. I noted that once we drop logicalrep_typmap_gettypname, there's really nothing using the LogicalRepTypMap table at all, so I nuked that too. (We can always recover the code from the git archives if some reason to use it emerges.) Didn't look at 0002 yet. regards, tom lane
Re: Synchronous commit behavior during network outage
On Fri, 2021-07-02 at 11:39 +0500, Andrey Borodin wrote: > If the failover happens due to unresponsive node we cannot just turn > off sync rep. We need to have some spare connections for that (number > of stuck backends will skyrocket during network partitioning). We > need available descriptors and some memory to fork new backend. We > will need to re-read config. We need time to try after all. > At some failures we may lack some of these. I think it's a good point that, when things start to go wrong, they can go very wrong very quickly. But until you've disabled sync rep, the primary will essentially be down for writes whether using this new feature or not. Even if you can terminate some backends to try to free space, the application will just make new connections that will get stuck the same way. You can avoid the "fork backend" problem by keeping a connection always open from the HA tool, or by editing the conf to disable sync rep and issuing SIGHUP instead. Granted, that still takes some memory. > Partial degradation is already hard task. Without ability to easily > terminate running Postgres HA tool will often resort to SIGKILL. When the system is really wedged as you describe (waiting on sync rep, tons of connections, and low memory), what information do you expect the HA tool to be able to collect, and what actions do you expect it to take? Presumably, you'd want it to disable sync rep at some point to get back online. Where does SIGTERM fit into the picture? > > If you don't handle the termination case, then there's still a > > chance > > for the transaction to become visible to other clients before its > > replicated. > > Termination is admin command, they know what they are doing. > Cancelation is part of user protocol. >From the pg_terminate_backend() docs: "This is also allowed if the calling role is a member of the role whose backend is being terminated or the calling role has been granted pg_signal_backend", so it's not really an admin command. Even for an admin, it might be hard to understand why terminating a backend could result in losing a visible transaction. I'm not really seeing two use cases here for two GUCs. Are you sure you want to disable only cancels but allow termination to proceed? Regards, Jeff Davis
Re: Add proper planner support for ORDER BY / DISTINCT aggregates
On 2/07/21 8:39 pm, David Rowley wrote: On Fri, 2 Jul 2021 at 19:54, Ronan Dunklau wrote: I don't know if it's acceptable, but in the case where you add both an aggregate with an ORDER BY clause, and another aggregate without the clause, the output for the unordered one will change and use the same ordering, maybe suprising the unsuspecting user. Would that be acceptable ? That's a good question. There was an argument in [1] that mentions that there might be a group of people who rely on aggregation being done in a certain order where they're not specifying an ORDER BY clause in the aggregate. If that group of people exists, then it's possible they might be upset in the scenario that you describe. [...] I've always worked on the assumption that if I do not specify an ORDER BY clause then the rdbms is expected to present rows in the order most efficient for it to do so. If pg orders rows when not requested then this could add extra elapsed time to the query, which might be significant for large queries. I don't know of any rdbms that guarantees the order of returned rows when an ORDER BY clause is not used. So I think that pg has no obligation to protect the sensibilities of naive users in this case, especially at the expense of users that want queries to complete as quickly as possible. IMHO Cheers, Gavin
Re: logical replication worker accesses catalogs in error context callback
I wrote:
> Didn't look at 0002 yet.
... and now that I have, I don't like it much. It adds a lot of
complexity, plus some lookup cycles that might be wasted. I experimented
with looking into the range table as I suggested previously, and
that seems to work; see attached. (This includes a little bit of
code cleanup along with the bug fix proper.)
An interesting point here is that the range table data will represent
table and column aliases, not necessarily their true names. I don't
find that wrong, it's just different from what the code presently
does. If we go with this, likely we should change the plain-relation
code path so that it also prints aliases from the RTE instead of
the actual names.
regards, tom lane
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..b40c331f8f 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -7185,6 +7185,10 @@ make_tuple_from_result_row(PGresult *res,
/*
* Callback function which is called when error occurs during column value
* conversion. Print names of column and relation.
+ *
+ * Note that this function mustn't do any catalog lookups, since we are in
+ * an already-failed transaction. Fortunately, we can get info from the
+ * relcache entry (for a simple scan) or the query rangetable (for joins).
*/
static void
conversion_error_callback(void *arg)
@@ -7198,10 +7202,14 @@ conversion_error_callback(void *arg)
{
/* error occurred in a scan against a foreign table */
TupleDesc tupdesc = RelationGetDescr(errpos->rel);
- Form_pg_attribute attr = TupleDescAttr(tupdesc, errpos->cur_attno - 1);
if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
+ {
+ Form_pg_attribute attr = TupleDescAttr(tupdesc,
+ errpos->cur_attno - 1);
+
attname = NameStr(attr->attname);
+ }
else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
attname = "ctid";
@@ -7220,35 +7228,32 @@ conversion_error_callback(void *arg)
/*
* Target list can have Vars and expressions. For Vars, we can get
- * its relation, however for expressions we can't. Thus for
+ * some information, however for expressions we can't. Thus for
* expressions, just show generic context message.
*/
if (IsA(tle->expr, Var))
{
- RangeTblEntry *rte;
Var *var = (Var *) tle->expr;
+ RangeTblEntry *rte = exec_rt_fetch(var->varno, estate);
- rte = exec_rt_fetch(var->varno, estate);
+ relname = rte->eref->aliasname;
if (var->varattno == 0)
is_wholerow = true;
- else
-attname = get_attname(rte->relid, var->varattno, false);
-
- relname = get_rel_name(rte->relid);
+ else if (var->varattno > 0 &&
+ var->varattno <= list_length(rte->eref->colnames))
+attname = strVal(list_nth(rte->eref->colnames,
+ var->varattno - 1));
}
- else
- errcontext("processing expression at position %d in select list",
- errpos->cur_attno);
}
- if (relname)
- {
- if (is_wholerow)
- errcontext("whole-row reference to foreign table \"%s\"", relname);
- else if (attname)
- errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
- }
+ if (relname && is_wholerow)
+ errcontext("whole-row reference to foreign table \"%s\"", relname);
+ else if (relname && attname)
+ errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
+ else
+ errcontext("processing expression at position %d in select list",
+ errpos->cur_attno);
}
/*
Re: Add proper planner support for ORDER BY / DISTINCT aggregates
Gavin Flower writes: > On 2/07/21 8:39 pm, David Rowley wrote: >> That's a good question. There was an argument in [1] that mentions >> that there might be a group of people who rely on aggregation being >> done in a certain order where they're not specifying an ORDER BY >> clause in the aggregate. If that group of people exists, then it's >> possible they might be upset in the scenario that you describe. > So I think that pg has no obligation to protect the sensibilities of > naive users in this case, especially at the expense of users that want > queries to complete as quickly as possible. IMHO I agree. We've broken such expectations in the past and I don't have much hesitation about breaking them again. regards, tom lane
Re: psql - factor out echo code
Fabien COELHO writes: > Yes. Maybe decorations should be SQL comments, and the purpose/origin of > the query could be made clear as you suggest, eg something like markdown > in a comment: >"-- # QUERY\n%s\n\n" If we keep the decoration, I'd agree with dropping all the asterisks. I'd vote for something pretty minimalistic, like -- INTERNAL QUERY: regards, tom lane
Re: psql - factor out echo code
On 2021-Jul-02, Tom Lane wrote: > Fabien COELHO writes: > > Yes. Maybe decorations should be SQL comments, and the purpose/origin of > > the query could be made clear as you suggest, eg something like markdown > > in a comment: > >"-- # QUERY\n%s\n\n" > > If we keep the decoration, I'd agree with dropping all the asterisks. > I'd vote for something pretty minimalistic, like > > -- INTERNAL QUERY: I think the most interesting case for decoration is the "step by step" mode, where you want the "title" that precedes each query be easily visible. I think two uppercase words are not sufficient for that ... and Markdown format which would force you to convert to HTML before you can notice where it is, are not sufficient either. The line with a few asterisks seems fine to me for that. Removing the asterisks in the other case seems fine. I admit I don't use the step-by-step mode all that much, though. Also: one place that prints queries that wasn't mentioned before is exec_command_print() in command.c. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "Ed is the standard text editor." http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3
Re: Fix uninitialized variable access (src/backend/utils/mmgr/freepage.c)
Em qui., 1 de jul. de 2021 às 17:20, Mahendra Singh Thalor < mahi6...@gmail.com> escreveu: > On Fri, 2 Jul 2021 at 01:13, Ranier Vilela wrote: > > > > Hi, > > > > The function FreePageManagerPutInternal can access an uninitialized > variable, > > if the following conditions occur: > > Patch looks good to me. > > > 1. fpm->btree_depth != 0 > > 2. relptr_off == 0 inside function (FreePageBtreeSearch) > > > > Perhaps this is a rare situation, but I think it's worth preventing. > > Please can we try to hit this rare condition by any test case. If you have > any test cases, please share. > Added to Commitfest (https://commitfest.postgresql.org/34/3236/), so we don't forget. regards, Ranier Vilela >
Re: psql - factor out echo code
Alvaro Herrera writes: > I think the most interesting case for decoration is the "step by step" > mode, where you want the "title" that precedes each query be easily > visible. I'm okay with leaving the step-by-step prompt as-is, personally. It's the inconsistency of the other ones that bugs me. > Also: one place that prints queries that wasn't mentioned before is > exec_command_print() in command.c. Ah, I was wondering if anyplace outside common.c did so. But that one seems to me to be a different animal -- it's not logging queries-about-to-be-executed. regards, tom lane
Re: psql - factor out echo code
"-- # QUERY\n%s\n\n"
Attached an attempt along those lines. I found another duplicate of the
ascii-art printing in another function.
Completion queries seems to be out of the echo/echo hidden feature.
Incredible, there is a (small) impact on regression tests for the \gexec
case. All other changes have no impact, because they are not tested:-(
--
Fabien.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 543401c6d6..8ec00881db 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2061,7 +2061,7 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD ",
fmtId(user));
appendStringLiteralConn(&buf, encrypted_password, pset.db);
-res = PSQLexec(buf.data);
+res = PSQLexec("PASSWORD", buf.data);
termPQExpBuffer(&buf);
if (!res)
success = false;
@@ -4993,22 +4993,13 @@ do_watch(PQExpBuffer query_buf, double sleep)
* returns true unless we have ECHO_HIDDEN_NOEXEC.
*/
static bool
-echo_hidden_command(const char *query)
+echo_hidden_command(const char *origin, const char *query)
{
if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
{
- printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
- fflush(stdout);
+ echoQuery(stdout, origin, query);
if (pset.logfile)
- {
- fprintf(pset.logfile,
- _("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
- fflush(pset.logfile);
- }
+ echoQuery(pset.logfile, origin, query);
if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
return false;
@@ -5060,7 +5051,7 @@ lookup_object_oid(EditableObjectType obj_type, const char *desc,
break;
}
- if (!echo_hidden_command(query->data))
+ if (!echo_hidden_command("INTERNAL", query->data))
{
destroyPQExpBuffer(query);
return false;
@@ -5155,7 +5146,7 @@ get_create_object_cmd(EditableObjectType obj_type, Oid oid,
break;
}
- if (!echo_hidden_command(query->data))
+ if (!echo_hidden_command("INTERNAL", query->data))
{
destroyPQExpBuffer(query);
return false;
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9a00499510..69d4e33093 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -523,6 +523,17 @@ PrintTiming(double elapsed_msec)
elapsed_msec, days, (int) hours, (int) minutes, seconds);
}
+/*
+ * Echo user query
+ */
+void
+echoQuery(FILE *out, const char *origin, const char *query)
+{
+ fprintf(out,
+ /* FIXME should this really be translatable? */
+ _("-- # %s QUERY\n%s\n\n"), origin, query);
+ fflush(out);
+}
/*
* PSQLexec
@@ -537,7 +548,7 @@ PrintTiming(double elapsed_msec)
* caller uses this path to issue "SET CLIENT_ENCODING".
*/
PGresult *
-PSQLexec(const char *query)
+PSQLexec(const char *origin, const char *query)
{
PGresult *res;
@@ -549,18 +560,9 @@ PSQLexec(const char *query)
if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
{
- printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
- fflush(stdout);
+ echoQuery(stdout, origin, query);
if (pset.logfile)
- {
- fprintf(pset.logfile,
- _("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
- fflush(pset.logfile);
- }
+ echoQuery(pset.logfile, origin, query);
if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
return NULL;
@@ -856,10 +858,7 @@ ExecQueryTuples(const PGresult *result)
* assumes that MainLoop did that, so we have to do it here.
*/
if (pset.echo == PSQL_ECHO_ALL && !pset.singlestep)
-{
- puts(query);
- fflush(stdout);
-}
+ echoQuery(stdout, "GEXEC", query);
if (!SendQuery(query))
{
@@ -1226,13 +1225,7 @@ SendQuery(const char *query)
}
if (pset.logfile)
- {
- fprintf(pset.logfile,
-_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
- fflush(pset.logfile);
- }
+ echoQuery(pset.logfile, "USER", query);
SetCancelConn(pset.db);
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 041b2ac068..cdad55414a 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -28,9 +28,10 @@ extern sigjmp_buf sigint_interrupt_jmp;
extern void psql_setup_cancel_handler(void);
-extern PGresult *PSQLexec(const char *query);
+extern PGresult *PSQLexec(const char *origin, const char *query);
extern int PSQLexecWatch(const char *query, const printQueryOpt *opt);
+extern void echoQuery(FILE *out, const char *origin, const char *query);
extern bool SendQuery(const char *query);
extern bool is_superuser(void);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2abf255798..b7f701a68a 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -127,7 +127,7 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
appendPQExpBufferStr(&buf, "ORDER
Re: rand48 replacement
Hello Dean, It may be true that the bias is of the same magnitude as FP multiply, but it is not of the same nature. With FP multiply, the more-likely-to-be-chosen values are more-or-less evenly distributed across the range, whereas modulo concentrates them all at one end, making it more likely to bias test results. Yes, that is true. It's worth paying attention to how other languages/libraries implement this, and basically no one chooses the modulo method, which ought to raise alarm bells. Of the biased methods, it has the worst kind of bias and the worst performance. Hmmm. That is not exactly how I interpreted the figures in the blog. If a biased method is OK, then the biased integer multiply method seems to be the clear winner. This requires the high part of a 64x64-bit product, which is trivial if 128-bit integers are available, but would need a little more work otherwise. There's some code in common/d2s that might be suitable. And yes, modulo is expensive. If we allow 128 bits integers operations, I would not choose this RNPG in the first place, I'd take PCG with a 128 bits state. That does not change the discussion about bias, though. Most other implementations tend to use an unbiased method though, and I think it's worth doing the same. It might be a bit slower, or even faster depending on implementation and platform, but in the context of the DB as a whole, I don't think a few extra cycles matters either way. Ok ok ok, I surrender! The method recommended at the very end of that blog seems to be pretty good all round, but any of the other commonly used unbiased methods would probably be OK too. That does not address my other issues with the proposed methods, in particular the fact that the generated sequence is less deterministic, but I think I have a simple way around that. I'm hesitating to skip to the the bitmask method, and give up performance uniformity. I'll try to come up with something over the week-end, and also address Tom's comments in passing. -- Fabien.
Re: Preventing abort() and exit() calls in libpq
On Wed, 2021-06-30 at 10:42 -0400, Tom Lane wrote: > Alvaro Herrera writes: > > On 2021-Jun-30, Tom Lane wrote: > > > You mentioned __gcov_exit, but I'm not sure if we need an > > > exception for that. I see it referenced by the individual .o > > > files, but the completed .so has no such reference, so at least > > > on RHEL8 it's apparently satisfied during .so linkage. Do you > > > see something different? > > Well, not really. I saw it but only after I removed -fprofile-arcs from > > Makefile.shlib's link line; but per my other email, that doesn't really > > work. > > Everything seems to work well for me after removing abort from that grep. > > OK, thanks, will push a fix momentarily. With latest HEAD, building with --enable-coverage still fails on my Ubuntu 20.04: ! nm -A -u libpq.so.5.15 2>/dev/null | grep -v __cxa_atexit | grep exit libpq.so.5.15: U exit@@GLIBC_2.2.5 I don't see any exit references in the libpq objects or in libpgport_shlib, so it seems like libpgcommon_shlib is the culprit... I assume turning off optimizations leads to less dead code elimination? --Jacob
Re: Preventing abort() and exit() calls in libpq
Jacob Champion writes: > With latest HEAD, building with --enable-coverage still fails on my > Ubuntu 20.04: > ! nm -A -u libpq.so.5.15 2>/dev/null | grep -v __cxa_atexit | grep exit > libpq.so.5.15: U exit@@GLIBC_2.2.5 Hm, weird. I don't see that here on RHEL8, and https://coverage.postgresql.org seems to be working so it doesn't fail on Alvaro's Debian setup either. What configure options are you using? Does "nm -u" report "exit" being referenced from any *.o in libpq, or from any *_shlib.o in src/port/ or src/common/ ? regards, tom lane
Re: Preventing abort() and exit() calls in libpq
On Fri, 2021-07-02 at 18:20 -0400, Tom Lane wrote: > What configure options are you using? Just `./configure --enable-coverage`, nothing else. I distclean'd right before for good measure. > Does "nm -u" report "exit" being referenced from any *.o in libpq, > or from any *_shlib.o in src/port/ or src/common/ ? Only src/common: controldata_utils_shlib.o: U close U __errno_location U exit ... fe_memutils_shlib.o: U exit ... file_utils_shlib.o: U close U closedir U __errno_location U exit ... hex_shlib.o: U exit ... psprintf_shlib.o: U __errno_location U exit ... stringinfo_shlib.o: U __errno_location U exit ... username_shlib.o: U __errno_location U exit ... --Jacob
Re: Preventing abort() and exit() calls in libpq
Jacob Champion writes: > On Fri, 2021-07-02 at 18:20 -0400, Tom Lane wrote: >> What configure options are you using? > Just `./configure --enable-coverage`, nothing else. I distclean'd right > before for good measure. Hmph. There's *something* different about your setup from what either Alvaro or I tried. What's the compiler (and version)? What's the platform exactly? regards, tom lane
Re: Preventing abort() and exit() calls in libpq
On Fri, 2021-07-02 at 18:45 -0400, Tom Lane wrote: > Jacob Champion writes: > > On Fri, 2021-07-02 at 18:20 -0400, Tom Lane wrote: > > > What configure options are you using? > > Just `./configure --enable-coverage`, nothing else. I distclean'd right > > before for good measure. > > Hmph. There's *something* different about your setup from what > either Alvaro or I tried. What's the compiler (and version)? > What's the platform exactly? $ gcc --version gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 Copyright (C) 2019 Free Software Foundation, Inc. ... $ cat /etc/os-release NAME="Ubuntu" VERSION="20.04.2 LTS (Focal Fossa)" ID=ubuntu ID_LIKE=debian PRETTY_NAME="Ubuntu 20.04.2 LTS" VERSION_ID="20.04" ... $ uname -a Linux HOSTNAME 5.8.0-59-generic #66~20.04.1-Ubuntu SMP Thu Jun 17 11:14:10 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux --Jacob
Re: Preventing abort() and exit() calls in libpq
On 2021-Jul-02, Jacob Champion wrote: > Only src/common: > > controldata_utils_shlib.o: > U close > U __errno_location > U exit Actually, I do see these in the .o files as well, but they don't make it to the .a file. gcc here is 8.3.0. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Numeric multiplication overflow errors
On Fri, 2 Jul 2021 at 12:56, David Rowley wrote: > > On Fri, 2 Jul 2021 at 22:55, Dean Rasheed wrote: > > Here's an update with the > > last set of changes discussed. > > Looks good to me. Thanks for the review and testing! > Just the question of if we have any problems changing the serialized > format in the back branches. I'm not sure if that's something we've > done before. I only had a quick look of git blame in the > serial/deserial functions and the only changes I really see apart from > a few cosmetic ones were a57d312a7 and 9cca11c91. Both of which just > went into master. Thinking about this more, I think it's best not to risk back-patching. It *might* be safe, but it's difficult to really be sure of that. The bug itself is pretty unlikely to ever happen in practice, hence the lack of prior complaints, and in fact I only found it by an examination of the code. So it doesn't seem to be worth the risk. OTOH, the original bug, with numeric *, is one I have hit in practice, and the fix is trivial and low risk, so I would like to backpatch that fix. Regards, Dean
Re: Preventing abort() and exit() calls in libpq
"alvhe...@alvh.no-ip.org" writes: > gcc here is 8.3.0. Hmmm ... mine is 8.4.1. I'm about to go out to dinner, but will check into this with some newer gcc versions later. regards, tom lane
Re: Preventing abort() and exit() calls in libpq
On Fri, Jul 02, 2021 at 11:20:17AM -0400, Tom Lane wrote: > Alvaro Herrera writes: > > Now it's hoverfly: > > ! nm -A -u libpq.so.5 2>/dev/null | grep -v __cxa_atexit | grep exit > > libpq.so.5: atexit U - > > libpq.so.5: pthread_exit U - > > Ugh. What in the world is producing those references? Those come from a statically-linked libldap_r: $ nm -A -u /home/nm/sw/nopath/openldap-64/lib/libldap_r.a|grep exit /home/nm/sw/nopath/openldap-64/lib/libldap_r.a[tpool.o]: .ldap_pvt_thread_exit U - /home/nm/sw/nopath/openldap-64/lib/libldap_r.a[thr_posix.o]: .pthread_exit U - /home/nm/sw/nopath/openldap-64/lib/libldap_r.a[init.o]: .atexit U -
Re: logical replication worker accesses catalogs in error context callback
On Sat, Jul 3, 2021 at 1:37 AM Tom Lane wrote: > > Bharath Rupireddy writes: > >> << Attaching v5-0001 here again for completion >> > >> I'm attaching v5-0001 patch that avoids printing the column type names > >> in the context message thus no cache lookups have to be done in the > >> error context callback. I think the column name is enough to know on > >> which column the error occurred and if required it's type can be known > >> by the user. This patch gets rid of printing local and remote type > >> names in slot_store_error_callback and also > >> logicalrep_typmap_gettypname because it's unnecessary. Thoughts? > > I've now pushed this patch. I noted that once we drop > logicalrep_typmap_gettypname, there's really nothing using > the LogicalRepTypMap table at all, so I nuked that too. > (We can always recover the code from the git archives if > some reason to use it emerges.) Isn't it better to have the below comment before slot_store_error_callback, similar to what's there before conversion_error_callback in v7-0002. * Note that this function mustn't do any catalog lookups, since we are in * an already-failed transaction. Maybe it's worth considering avoid_sys_cache_lookup_in_error_callback_v2.patch from [1] as another way to enforce the above statement. [1] - https://www.postgresql.org/message-id/CAD21AoAwxbd-zXXUAeJ2FBRHr%2B%3DbfMUHoN7xJuXcwu1sFi1-sQ%40mail.gmail.com Regards, Bharath Rupireddy.
Re: logical replication worker accesses catalogs in error context callback
On Sat, Jul 3, 2021 at 2:19 AM Tom Lane wrote:
>
> I wrote:
> > Didn't look at 0002 yet.
>
> ... and now that I have, I don't like it much. It adds a lot of
> complexity, plus some lookup cycles that might be wasted. I experimented
> with looking into the range table as I suggested previously, and
> that seems to work; see attached. (This includes a little bit of
> code cleanup along with the bug fix proper.)
>
> An interesting point here is that the range table data will represent
> table and column aliases, not necessarily their true names. I don't
> find that wrong, it's just different from what the code presently
> does. If we go with this, likely we should change the plain-relation
> code path so that it also prints aliases from the RTE instead of
> the actual names.
Thanks. This patch is way simpler than what I proposed. Also, I like
the generic message "processing expression at position %d in select
list" when relname or attname is not available.
The patch basically looks good to me except a minor comment to have a
local variable for var->varattno which makes the code shorter.
if (IsA(tle->expr, Var))
{
- RangeTblEntry *rte;
Var*var = (Var *) tle->expr;
+ AttrNumber attno = var->varattno;
+ RangeTblEntry *rte = exec_rt_fetch(var->varno, estate);
- rte = exec_rt_fetch(var->varno, estate);
+ relname = rte->eref->aliasname;
- if (var->varattno == 0)
+ if (attno == 0)
is_wholerow = true;
- else
- attname = get_attname(rte->relid,
var->varattno, false);
-
- relname = get_rel_name(rte->relid);
+ else if (attno > 0 && attno <=
list_length(rte->eref->colnames))
+ attname =
strVal(list_nth(rte->eref->colnames, attno - 1));
}
Regards,
Bharath Rupireddy.
Re: Logical replication - schema change not invalidating the relation cache
On Fri, Jul 2, 2021 at 12:03 PM Dilip Kumar wrote: > > Yeah, this looks like a bug. I will look at the patch. > While looking into this, I think the main cause of the problem is that schema rename does not invalidate the relation cache right? I also tried other cases e.g. if there is an open cursor and we rename the schema CREATE SCHEMA sch1; CREATE TABLE sch1.t1(c1 int); insert into sch1.t1 values(1); insert into sch1.t1 values(2); insert into sch1.t1 values(3); BEGIN; DECLARE mycur CURSOR FOR SELECT * FROM sch1.t1; FETCH NEXT FROM mycur ; --At this point rename sch1 to sch2 from another session-- FETCH NEXT FROM mycur ; UPDATE sch2.t1 SET c1 = 20 WHERE CURRENT OF mycur; select * from sch2.t1 ; So even after the schema rename the cursor is able to fetch and its also able to update on the same table in the new schema, ideally using CURRENT OF CUR, you can update the same table for which you have declared the cursor. I am giving this example because this behavior also looks somewhat similar. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
