bug: copy progress reporting of backends which run multiple COPYs
pg_stat_progress_copy was added in v14 (8a4f618e7, 9d2d45700).
But if a command JOINs file_fdw tables, the progress report gets bungled
up. This will warn/assert during file_fdw tests.
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index 6743e68cef6..7abcb4f60db 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -10,6 +10,7 @@
*/
#include "postgres.h"+#include "commands/progress.h"
#include "port/atomics.h" /* for memory barriers */
#include "utils/backend_progress.h"
#include "utils/backend_status.h"
@@ -105,6 +106,20 @@ pgstat_progress_end_command(void)
if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID)
return;
+// This currently fails file_fdw tests, since pgstat_progress evidently fails
+// to support simultaneous copy commands, as happens during JOIN.
+ /* bytes progress is not available in all cases */
+ if (beentry->st_progress_command == PROGRESS_COMMAND_COPY &&
+ beentry->st_progress_param[PROGRESS_COPY_BYTES_TOTAL] > 0)
+ {
+ volatile int64 *a = beentry->st_progress_param;
+ if (a[PROGRESS_COPY_BYTES_PROCESSED] > a[PROGRESS_COPY_BYTES_TOTAL])
+ elog(WARNING, "PROGRESS_COPY_BYTES_PROCESSED %ld %ld",
+ a[PROGRESS_COPY_BYTES_PROCESSED],
+ a[PROGRESS_COPY_BYTES_TOTAL]);
+ // Assert(a[PROGRESS_COPY_BYTES_PROCESSED] <= a[PROGRESS_COPY_BYTES_TOTAL]);
+ }
+
PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
beentry->st_progress_command_target = InvalidOid;
On Thu, 19 Jan 2023 at 06:47, Justin Pryzby <pryzby@telsasoft.com> wrote:
pg_stat_progress_copy was added in v14 (8a4f618e7, 9d2d45700).
But if a command JOINs file_fdw tables, the progress report gets bungled
up. This will warn/assert during file_fdw tests.
I don't know what to do with that other than disabling COPY progress
reporting for file_fdw, i.e. calls to BeginCopyFrom that don't supply
a pstate. This is probably the best option, because a table backed by
file_fdw would also interfere with COPY TO's progress reporting.
Attached a patch that solves this specific issue in a
binary-compatible way. I'm not super happy about relying on behavior
of callers of BeginCopyFrom (assuming that users that run copy
concurrently will not provide a ParseState* to BeginCopyFrom), but it
is what it is.
Kind regards,
Matthias van de Meent
Attachments:
v1-0001-Only-report-progress-if-we-re-actually-in-a-parse.patchapplication/octet-stream; name=v1-0001-Only-report-progress-if-we-re-actually-in-a-parse.patchDownload
From bf31258d590bbac6bf243a2eafcd71272f79ccf7 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Sat, 21 Jan 2023 01:46:52 +0100
Subject: [PATCH v1] Only report progress if we're actually in a (parsed) COPY
command
We assume that only exclusive COPY commands (including table sync
worker) provide a pstate to BeginCopyFrom.
This solves bogus progress reporting stats in file_fdw, which uses
the copy infrastructure for implementing a foreign data wrapper, and
may thus have more than one copy in progress at the same time.
---
src/backend/commands/copyfrom.c | 35 ++++++++++++++++--------
src/include/commands/copyfrom_internal.h | 1 +
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index af52faca6d..c785ac49bc 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -382,8 +382,9 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
/* Update the row counter and progress of the COPY command */
*processed += inserted;
- pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
- *processed);
+ if (cstate->report_progress)
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
+ *processed);
}
for (i = 0; i < nused; i++)
@@ -461,8 +462,9 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
/* Update the row counter and progress of the COPY command */
*processed += nused;
- pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
- *processed);
+ if (cstate->report_progress)
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
+ *processed);
/* reset cur_lineno and line_buf_valid to what they were */
cstate->line_buf_valid = line_buf_valid;
@@ -1016,8 +1018,10 @@ CopyFrom(CopyFromState cstate)
* Report that this tuple was filtered out by the WHERE
* clause.
*/
- pgstat_progress_update_param(PROGRESS_COPY_TUPLES_EXCLUDED,
- ++excluded);
+ ++excluded;
+ if (cstate->report_progress)
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_EXCLUDED,
+ excluded);
continue;
}
}
@@ -1271,8 +1275,10 @@ CopyFrom(CopyFromState cstate)
* for counting tuples inserted by an INSERT command. Update
* progress of the COPY command as well.
*/
- pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
- ++processed);
+ ++processed;
+ if (cstate->report_progress)
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED,
+ processed);
}
}
@@ -1384,6 +1390,8 @@ BeginCopyFrom(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
+ cstate->report_progress = pstate != NULL;
+
/* Process the target relation */
cstate->rel = rel;
@@ -1610,8 +1618,9 @@ BeginCopyFrom(ParseState *pstate,
/* initialize progress */
- pgstat_progress_start_command(PROGRESS_COMMAND_COPY,
- cstate->rel ? RelationGetRelid(cstate->rel) : InvalidOid);
+ if (cstate->report_progress)
+ pgstat_progress_start_command(PROGRESS_COMMAND_COPY,
+ cstate->rel ? RelationGetRelid(cstate->rel) : InvalidOid);
cstate->bytes_processed = 0;
/* We keep those variables in cstate. */
@@ -1687,7 +1696,8 @@ BeginCopyFrom(ParseState *pstate,
}
}
- pgstat_progress_update_multi_param(3, progress_cols, progress_vals);
+ if (cstate->report_progress)
+ pgstat_progress_update_multi_param(3, progress_cols, progress_vals);
if (cstate->opts.binary)
{
@@ -1729,7 +1739,8 @@ EndCopyFrom(CopyFromState cstate)
cstate->filename)));
}
- pgstat_progress_end_command();
+ if (cstate->report_progress)
+ pgstat_progress_end_command();
MemoryContextDelete(cstate->copycontext);
pfree(cstate);
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index 7b1c4327bd..4b8782d481 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -66,6 +66,7 @@ typedef struct CopyFromStateData
EolType eol_type; /* EOL type of input */
int file_encoding; /* file or remote side's character encoding */
bool need_transcoding; /* file encoding diff from server? */
+ bool report_progress; /* do we need to report progress? */
Oid conversion_proc; /* encoding conversion function */
/* parameters from the COPY command */
--
2.39.0
On Fri, Jan 20, 2023 at 4:51 PM Matthias van de Meent <
boekewurm+postgres@gmail.com> wrote:
On Thu, 19 Jan 2023 at 06:47, Justin Pryzby <pryzby@telsasoft.com> wrote:
pg_stat_progress_copy was added in v14 (8a4f618e7, 9d2d45700).
But if a command JOINs file_fdw tables, the progress report gets bungled
up. This will warn/assert during file_fdw tests.I don't know what to do with that other than disabling COPY progress
reporting for file_fdw, i.e. calls to BeginCopyFrom that don't supply
a pstate. This is probably the best option, because a table backed by
file_fdw would also interfere with COPY TO's progress reporting.Attached a patch that solves this specific issue in a
binary-compatible way. I'm not super happy about relying on behavior
of callers of BeginCopyFrom (assuming that users that run copy
concurrently will not provide a ParseState* to BeginCopyFrom), but it
is what it is.Kind regards,
Matthias van de Meent
Hi,
In `BeginCopyFrom`, I see the following :
if (pstate)
{
cstate->range_table = pstate->p_rtable;
cstate->rteperminfos = pstate->p_rteperminfos;
Is it possible to check range_table / rteperminfos so that we don't
introduce the bool field ?
Cheers
On Sat, 21 Jan 2023 at 02:04, Ted Yu <yuzhihong@gmail.com> wrote:
On Fri, Jan 20, 2023 at 4:51 PM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
Attached a patch that solves this specific issue in a
binary-compatible way. I'm not super happy about relying on behavior
of callers of BeginCopyFrom (assuming that users that run copy
concurrently will not provide a ParseState* to BeginCopyFrom), but it
is what it is.Is it possible to check range_table / rteperminfos so that we don't introduce the bool field ?
I think yes, but I'm not sure we can depend on rteperminfos to be set,
and the same for p_rtable. I also don't think it's a good idea for
code clarity: there is no good reason why the (un)availability of
either range_table or rteperminfos would allow progress reporting - it
would require additional extensive documentation around both the usage
sites and the field itself. Adding a well-named field provides a much
better experience in my opinion.
If someone were opposed to adding that field in backbranches I'm fine
with using one of these instead, assuming additional clear
documentation is added as well.
- Matthias
On Sat, Jan 21, 2023 at 01:51:28AM +0100, Matthias van de Meent wrote:
On Thu, 19 Jan 2023 at 06:47, Justin Pryzby <pryzby@telsasoft.com> wrote:
pg_stat_progress_copy was added in v14 (8a4f618e7, 9d2d45700).
But if a command JOINs file_fdw tables, the progress report gets bungled
up. This will warn/assert during file_fdw tests.I don't know what to do with that other than disabling COPY progress
reporting for file_fdw, i.e. calls to BeginCopyFrom that don't supply
a pstate. This is probably the best option, because a table backed by
file_fdw would also interfere with COPY TO's progress reporting.Attached a patch that solves this specific issue in a
binary-compatible way. I'm not super happy about relying on behavior
of callers of BeginCopyFrom (assuming that users that run copy
concurrently will not provide a ParseState* to BeginCopyFrom), but it
is what it is.
Thanks for looking. Maybe another option is to avoid progress reporting
in 2nd and later CopyFrom() if another COPY was already running in that
backend.
Would you do anything different in the master branch, with no
compatibility constraints ? I think the progress reporting would still
be limited to one row per backend, not one per CopyFrom().
--
Justin
On Sat, 21 Jan 2023 at 02:28, Justin Pryzby <pryzby@telsasoft.com> wrote:
On Sat, Jan 21, 2023 at 01:51:28AM +0100, Matthias van de Meent wrote:
On Thu, 19 Jan 2023 at 06:47, Justin Pryzby <pryzby@telsasoft.com> wrote:
pg_stat_progress_copy was added in v14 (8a4f618e7, 9d2d45700).
But if a command JOINs file_fdw tables, the progress report gets bungled
up. This will warn/assert during file_fdw tests.I don't know what to do with that other than disabling COPY progress
reporting for file_fdw, i.e. calls to BeginCopyFrom that don't supply
a pstate. This is probably the best option, because a table backed by
file_fdw would also interfere with COPY TO's progress reporting.Attached a patch that solves this specific issue in a
binary-compatible way. I'm not super happy about relying on behavior
of callers of BeginCopyFrom (assuming that users that run copy
concurrently will not provide a ParseState* to BeginCopyFrom), but it
is what it is.Thanks for looking. Maybe another option is to avoid progress reporting
in 2nd and later CopyFrom() if another COPY was already running in that
backend.
Let me think about it. I think it would work, but I'm not sure that's
a great option - it adds backend state that we would need to add
cleanup handles for. But then again, COPY ... TO could use TRIGGER to
trigger actual COPY FROM statements, which would also break progress
reporting in a vanilla instance without extensions.
I'm not sure what the right thing to do is here.
Would you do anything different in the master branch, with no
compatibility constraints ? I think the progress reporting would still
be limited to one row per backend, not one per CopyFrom().
I think I would at least introduce another parameter to BeginCopyFrom
for progress reporting (instead of relying on pstate != NULL), like
how we have a bit in reindex_index's params->options that specifies
whether we want progress reporting (which is unset for parallel
workers iirc).
- Matthias
On Sat, Jan 21, 2023 at 02:45:40AM +0100, Matthias van de Meent wrote:
Let me think about it. I think it would work, but I'm not sure that's
a great option - it adds backend state that we would need to add
cleanup handles for. But then again, COPY ... TO could use TRIGGER to
trigger actual COPY FROM statements, which would also break progress
reporting in a vanilla instance without extensions.I'm not sure what the right thing to do is here.
Simply disabling COPY reporting for file_fdw does not sound appealing
to me, because that can be really useful for users. As long as you
rely on two code paths that call separately the progress reporting,
things are doomed with the current infrastructure. For example,
thinking about some fancy cases, you could you create an index that
uses a function as expression, function that includes utility commands
that do progress reporting. Things would equally go wrong in the
progress view.
What are the assertions/warnings that get triggered in file_fdw?
Joining together file_fdw with a plain COPY is surely a fancy case,
even if COPY TO would allow that.
Would you do anything different in the master branch, with no
compatibility constraints ? I think the progress reporting would still
be limited to one row per backend, not one per CopyFrom().I think I would at least introduce another parameter to BeginCopyFrom
for progress reporting (instead of relying on pstate != NULL), like
how we have a bit in reindex_index's params->options that specifies
whether we want progress reporting (which is unset for parallel
workers iirc).
This makes sense, independently of the discussion about what should be
done with cross-runs of these APIs. This could be extended with
user-controllable options for each one of them. It does not take care
of the root of the problem, just bypasses it.
--
Michael
On Sat, Jan 21, 2023 at 02:45:40AM +0100, Matthias van de Meent wrote:
Would you do anything different in the master branch, with no
compatibility constraints ? I think the progress reporting would still
be limited to one row per backend, not one per CopyFrom().I think I would at least introduce another parameter to BeginCopyFrom
for progress reporting (instead of relying on pstate != NULL), like
how we have a bit in reindex_index's params->options that specifies
whether we want progress reporting (which is unset for parallel
workers iirc).
This didn't get fixed for v16, and it seems unlikely that it'll be
addressed in back branches.
But while I was reviewing forgotten threads, it occurred to me to raise
the issue in time to fix it for v17.
--
Justin
On Tue, May 07, 2024 at 07:27:54AM -0500, Justin Pryzby wrote:
This didn't get fixed for v16, and it seems unlikely that it'll be
addressed in back branches.But while I was reviewing forgotten threads, it occurred to me to raise
the issue in time to fix it for v17.
Thanks for the reminder.
FWIW, I'm rather annoyed by the fact that we rely on the ParseState to
decide if reporting should happen or not. file_fdw tells, even if
that's accidental, that status reporting can be useful if working on a
single table. So, shutting down the whole reporting if a caller if
BeginCopyFrom() does not give a ParseState is too heavy-handed, IMO.
The addition of report_progress in the COPY FROM state data is a good
idea, though isn't that something we should give as an argument of
BeginCopyFrom() instead if sticking this knowledge in COPY FROM?
Different idea: could it be something worth controlling with a
query-level option? It would then be possible to provide the same
level of control for COPY TO which has reporting paths, given the
application control over the reporting even with file_fdw, and store
the value controlling the reporting in CopyFormatOptions. I am
wondering if there would be a case where someone wants to do a COPY
but hide entirely the reporting done.
The query-level option has some appeal.
--
Michael
On Tue, May 7, 2024 at 9:12 PM Michael Paquier <michael@paquier.xyz> wrote:
FWIW, I'm rather annoyed by the fact that we rely on the ParseState to
decide if reporting should happen or not. file_fdw tells, even if
that's accidental, that status reporting can be useful if working on a
single table. So, shutting down the whole reporting if a caller if
BeginCopyFrom() does not give a ParseState is too heavy-handed, IMO.
I think you're hoping for too much. The progress reporting
infrastructure is fundamentally designed around the idea that there
can only be one progress-reporting operation in progress at a time.
For COPY, that is, I believe, true, but for file_fdw, it's false. If
we want to do any kind of progress reporting from within plannable
queries, we need some totally different and much more complex
infrastructure that can report progress for, probably, each plan node
individually. I think it's likely a mistake to try to shoehorn cases
like this into the infrastructure
that we have today. It will just encourage people to try to use the
current infrastructure in ways that are less and less like what it was
actually designed to do; whereas what we should be doing if we want
this kind of functionality, at least IMHO, is building infrastructure
that's actually fit for purpose.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, May 08, 2024 at 10:07:15AM -0400, Robert Haas wrote:
I think you're hoping for too much. The progress reporting
infrastructure is fundamentally designed around the idea that there
can only be one progress-reporting operation in progress at a time.
For COPY, that is, I believe, true, but for file_fdw, it's false. If
we want to do any kind of progress reporting from within plannable
queries, we need some totally different and much more complex
infrastructure that can report progress for, probably, each plan node
individually. I think it's likely a mistake to try to shoehorn cases
like this into the infrastructure
that we have today. It will just encourage people to try to use the
current infrastructure in ways that are less and less like what it was
actually designed to do; whereas what we should be doing if we want
this kind of functionality, at least IMHO, is building infrastructure
that's actually fit for purpose.
Hmm. OK. I have been looking around for cases out there where
BeginCopyFrom() could be called with a pstate where the reporting
could matter, and could not find anything worth worrying about. It
still makes me a bit uneasy to not have a separate argument in the
function to control that. Now, if you, Justin and Matthias agree with
this approach, I won't stand in the way either.
--
Michael