public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v6 1/4] Make a message on process termination more dscriptive 5+ messages / 4 participants [nested] [flat]
* [PATCH v6 1/4] Make a message on process termination more dscriptive @ 2021-12-24 03:52 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2021-12-24 03:52 UTC (permalink / raw) The message at process termination due to slot limit doesn't provide the reason. In the major scenario the message is followed by another message about slot invalidatation, which shows the detail for the termination. However the second message is missing if the slot is temporary one. Augment the first message with the reason same as the second message. Backpatch through to 13 where the message was introduced. Reported-by: Alex Enachioaie <[email protected]> Author: Kyotaro Horiguchi <[email protected]> Reviewed-by: Ashutosh Bapat <[email protected]> Reviewed-by: Bharath Rupireddy <[email protected]> Discussion: https://www.postgresql.org/message-id/17327-89d0efa8b9ae6271%40postgresql.org Backpatch-through: 13 --- src/backend/replication/slot.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 8fec1cb4a5..8326c019cf 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1293,8 +1293,9 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlot *s, XLogRecPtr oldestLSN, if (last_signaled_pid != active_pid) { ereport(LOG, - (errmsg("terminating process %d to release replication slot \"%s\"", - active_pid, NameStr(slotname)))); + (errmsg("terminating process %d to release replication slot \"%s\" because its restart_lsn %X/%X exceeds max_slot_wal_keep_size", + active_pid, NameStr(slotname), + LSN_FORMAT_ARGS(restart_lsn)))); (void) kill(active_pid, SIGTERM); last_signaled_pid = active_pid; -- 2.31.1 ----Next_Part(Tue_Sep__6_14_53_36_2022_094)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v6-0002-Add-detailed-information-to-slot-invalidation-mes.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Generate pg_stat_get_xact*() functions with Macros @ 2023-01-12 18:24 Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2023-01-12 18:24 UTC (permalink / raw) To: Drouvot, Bertrand <[email protected]>; +Cc: Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On 2023-01-12 08:38:57 +0100, Drouvot, Bertrand wrote: > On 1/11/23 11:59 PM, Andres Freund wrote: > > > Now that this patch renames some fields > > > > I don't mind renaming the fields - the prefixes really don't provide anything > > useful. But it's not clear why this is related to this patch? You could just > > include the f_ prefix in the macro, no? > > > > > > Right, but the idea is to take the same approach that the one used in 8018ffbf58 (where placing the prefixes in the macro > would have been possible too). I'm not super happy about that patch tbh. > > Probably should remove PgStat_BackendFunctionEntry. > > I think that would be a 3rd patch, agree? Yep. > > I now see that PG_STAT_GET_DBENTRY_FLOAT8 already exists, defined the same > > way. But the name fields misleading enough that I'd be inclined to rename it? > > > > PG_STAT_GET_FUNCENTRY_FLOAT8_MS looks good by me. Waiting on what we'll decide for > the existing PG_STAT_GET_DBENTRY_FLOAT8 (so that I can align for the PG_STAT_GET_FUNCENTRY_FLOAT8). +1 > > Although I suspect this actually hints at an architectural thing that could be > > fixed better: Perhaps we should replace find_tabstat_entry() with a version > > returning a fully "reconciled" PgStat_StatTabEntry? It feels quite wrong to > > have that intimitate knowledge of the subtransaction stuff in pgstatfuncs.c > > and about how the different counts get combined. > > > > I think that'd allow us to move the definition of PgStat_TableStatus to > > PgStat_TableXactStatus, PgStat_TableCounts to pgstat_internal.h. Which feels a > > heck of a lot cleaner. > > Yeah, I think that would be for a 4th patch, agree? Yea. I am of multiple minds about the ordering. I can see benefits on fixing the architectural issue before reducing duplication in the accessor with a macro. The reason is that if we addressed the architectural issue, the difference between the xact and non-xact version will be very minimal, and could even be generated by the same macro. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Generate pg_stat_get_xact*() functions with Macros @ 2023-01-13 09:36 Drouvot, Bertrand <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Drouvot, Bertrand @ 2023-01-13 09:36 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On 1/12/23 7:24 PM, Andres Freund wrote: > Hi, > > On 2023-01-12 08:38:57 +0100, Drouvot, Bertrand wrote: >> On 1/11/23 11:59 PM, Andres Freund wrote: >>>> Now that this patch renames some fields >>> >>> I don't mind renaming the fields - the prefixes really don't provide anything >>> useful. But it's not clear why this is related to this patch? You could just >>> include the f_ prefix in the macro, no? >>> >>> >> >> Right, but the idea is to take the same approach that the one used in 8018ffbf58 (where placing the prefixes in the macro >> would have been possible too). > > I'm not super happy about that patch tbh. > > >>> Probably should remove PgStat_BackendFunctionEntry. >> >> I think that would be a 3rd patch, agree? > > Yep. > > > >>> I now see that PG_STAT_GET_DBENTRY_FLOAT8 already exists, defined the same >>> way. But the name fields misleading enough that I'd be inclined to rename it? >>> >> >> PG_STAT_GET_FUNCENTRY_FLOAT8_MS looks good by me. Waiting on what we'll decide for >> the existing PG_STAT_GET_DBENTRY_FLOAT8 (so that I can align for the PG_STAT_GET_FUNCENTRY_FLOAT8). > > +1 > > > >>> Although I suspect this actually hints at an architectural thing that could be >>> fixed better: Perhaps we should replace find_tabstat_entry() with a version >>> returning a fully "reconciled" PgStat_StatTabEntry? It feels quite wrong to >>> have that intimitate knowledge of the subtransaction stuff in pgstatfuncs.c >>> and about how the different counts get combined. >>> >>> I think that'd allow us to move the definition of PgStat_TableStatus to >>> PgStat_TableXactStatus, PgStat_TableCounts to pgstat_internal.h. Which feels a >>> heck of a lot cleaner. >> >> Yeah, I think that would be for a 4th patch, agree? > > Yea. I am of multiple minds about the ordering. I can see benefits on fixing > the architectural issue before reducing duplication in the accessor with a > macro. The reason is that if we addressed the architectural issue, the > difference between the xact and non-xact version will be very minimal, and > could even be generated by the same macro. > Yeah, I do agree and I'm in favor of this ordering: 1) replace find_tabstat_entry() with a version returning a fully "reconciled" PgStat_StatTabEntry 2) remove prefixes 3) Introduce the new macros And it looks to me that removing PgStat_BackendFunctionEntry can be done independently I'll first look at 1). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Generate pg_stat_get_xact*() functions with Macros @ 2023-01-13 18:37 Andres Freund <[email protected]> parent: Drouvot, Bertrand <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2023-01-13 18:37 UTC (permalink / raw) To: Drouvot, Bertrand <[email protected]>; +Cc: Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On 2023-01-13 10:36:49 +0100, Drouvot, Bertrand wrote: > > > > Although I suspect this actually hints at an architectural thing that could be > > > > fixed better: Perhaps we should replace find_tabstat_entry() with a version > > > > returning a fully "reconciled" PgStat_StatTabEntry? It feels quite wrong to > > > > have that intimitate knowledge of the subtransaction stuff in pgstatfuncs.c > > > > and about how the different counts get combined. > > > > > > > > I think that'd allow us to move the definition of PgStat_TableStatus to > > > > PgStat_TableXactStatus, PgStat_TableCounts to pgstat_internal.h. Which feels a > > > > heck of a lot cleaner. > > > > > > Yeah, I think that would be for a 4th patch, agree? > > > > Yea. I am of multiple minds about the ordering. I can see benefits on fixing > > the architectural issue before reducing duplication in the accessor with a > > macro. The reason is that if we addressed the architectural issue, the > > difference between the xact and non-xact version will be very minimal, and > > could even be generated by the same macro. > > > > Yeah, I do agree and I'm in favor of this ordering: > > 1) replace find_tabstat_entry() with a version returning a fully "reconciled" PgStat_StatTabEntry > 2) remove prefixes > 3) Introduce the new macros > I'll first look at 1). Makes sense. > And it looks to me that removing PgStat_BackendFunctionEntry can be done independently It's imo the function version of 1), just a bit simpler to implement due to the much simpler reconciliation. It could be done together with it, or separately. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Generate pg_stat_get_xact*() functions with Macros @ 2023-03-01 19:54 Gregory Stark (as CFM) <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Gregory Stark (as CFM) @ 2023-03-01 19:54 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]> Looks like you have a path forward on this and it's not ready to commit yet? In which case I'll mark it Waiting on Author? On Fri, 13 Jan 2023 at 13:38, Andres Freund <[email protected]> wrote: > > On 2023-01-13 10:36:49 +0100, Drouvot, Bertrand wrote: > > > I'll first look at 1). > > Makes sense. > > > And it looks to me that removing PgStat_BackendFunctionEntry can be done independently > > It's imo the function version of 1), just a bit simpler to implement due to > the much simpler reconciliation. It could be done together with it, or > separately. -- Gregory Stark As Commitfest Manager ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-03-01 19:54 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-12-24 03:52 [PATCH v6 1/4] Make a message on process termination more dscriptive Kyotaro Horiguchi <[email protected]> 2023-01-12 18:24 Re: Generate pg_stat_get_xact*() functions with Macros Andres Freund <[email protected]> 2023-01-13 09:36 ` Re: Generate pg_stat_get_xact*() functions with Macros Drouvot, Bertrand <[email protected]> 2023-01-13 18:37 ` Re: Generate pg_stat_get_xact*() functions with Macros Andres Freund <[email protected]> 2023-03-01 19:54 ` Re: Generate pg_stat_get_xact*() functions with Macros Gregory Stark (as CFM) <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox