public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v9 4/4] WIP: check tuple size
27+ messages / 3 participants
[nested] [flat]
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v10 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index e018946c0e..ec9e43ac55 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -760,7 +760,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--8nsIa27JVQLqB7/C--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v8 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 47a5271b91..b4f523e32d 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--AZuoSAvZwvV/ife4--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v9 4/4] WIP: check tuple size
@ 2020-12-01 23:20 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Justin Pryzby @ 2020-12-01 23:20 UTC (permalink / raw)
Or maybe INSERT should flush buffer based only on the *number* of tuples, and
not their size ?
---
src/backend/executor/nodeModifyTable.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2059428e2a..04b0598183 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -703,7 +703,13 @@ ExecInsert(ModifyTableState *mtstate,
batchslot = MultiInsertInfoNextFreeSlot(mtstate->miinfo, resultRelInfo);
ExecCopySlot(batchslot, slot);
- MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot, 0, 0); // XXX: tuplen/lineno
+ MultiInsertInfoStore(mtstate->miinfo, resultRelInfo, batchslot,
+ // sizeof(void*) * batchslot->tts_nvalid, /* tuple size - underestimate */
+ MemoryContextMemAllocated(batchslot->tts_mcxt, true), /* tuple size */
+ mtstate->ntuples); /* lineno */
+
+ elog(DEBUG2, "bufferedBytes %d; tuples %ld",
+ mtstate->miinfo->bufferedBytes, mtstate->ntuples);
if (MultiInsertInfoIsFull(mtstate->miinfo))
MultiInsertInfoFlush(mtstate->miinfo, resultRelInfo);
--
2.17.0
--3yNHWXBV/QO9xKNm--
^ permalink raw reply [nested|flat] 27+ messages in thread
* walsender.c fileheader comment
@ 2024-06-11 02:35 Peter Smith <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Peter Smith @ 2024-06-11 02:35 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Hi,
I was reading the walsender.c fileheader comment while studying
another thread. I think if there is logical replication in progress
then the PROCSIG_WALSND_INIT_STOPPING handler will *always* switch to
a "stopping" state: e.g.,
/*
* Handle PROCSIG_WALSND_INIT_STOPPING signal.
*/
void
HandleWalSndInitStopping(void)
{
Assert(am_walsender);
/*
* If replication has not yet started, die like with SIGTERM. If
* replication is active, only set a flag and wake up the main loop. It
* will send any outstanding WAL, wait for it to be replicated to the
* standby, and then exit gracefully.
*/
if (!replication_active)
kill(MyProcPid, SIGTERM);
else
got_STOPPING = true;
}
~~~
But the walsender.c fileheader comment seems to be saying something
slightly different. IIUC, some minor rewording of the comment is
needed so it describes the code better.
HEAD
...
* shutdown, if logical replication is in progress all existing WAL records
* are processed followed by a shutdown. Otherwise this causes the walsender
* to switch to the "stopping" state. In this state, the walsender will reject
* any further replication commands. The checkpointer begins the shutdown
...
SUGGESTION
.. shutdown. If logical replication is in progress, the walsender
switches to a "stopping" state. In this state, the walsender will
reject any further replication commands - but all existing WAL records
are processed - followed by a shutdown.
~~~
I attached a patch for the above-suggested change.
Thoughts?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
[application/octet-stream] v1-0001-walsender-fileheader-comment.patch (2.0K, ../../CAHut+PusveZf5pfKTYzjioZ=Lx4TJFKRXs0KhTQjNueKQwkABg@mail.gmail.com/2-v1-0001-walsender-fileheader-comment.patch)
download | inline diff:
From 7188680f0f3ce81df6bf8067940b51257fbf3b5a Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Tue, 11 Jun 2024 12:31:05 +1000
Subject: [PATCH v1] walsender fileheader comment
---
src/backend/replication/walsender.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c623b07..1d41aea 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -27,14 +27,14 @@
* If the server is shut down, checkpointer sends us
* PROCSIG_WALSND_INIT_STOPPING after all regular backends have exited. If
* the backend is idle or runs an SQL query this causes the backend to
- * shutdown, if logical replication is in progress all existing WAL records
- * are processed followed by a shutdown. Otherwise this causes the walsender
- * to switch to the "stopping" state. In this state, the walsender will reject
- * any further replication commands. The checkpointer begins the shutdown
- * checkpoint once all walsenders are confirmed as stopping. When the shutdown
- * checkpoint finishes, the postmaster sends us SIGUSR2. This instructs
- * walsender to send any outstanding WAL, including the shutdown checkpoint
- * record, wait for it to be replicated to the standby, and then exit.
+ * shutdown. If logical replication is in progress, the walsender switches to a
+ * "stopping" state. In this state, the walsender will reject any further
+ * replication commands - but all existing WAL records are processed - followed
+ * by a shutdown. The checkpointer begins the shutdown checkpoint once all
+ * walsenders are confirmed as stopping. When the shutdown checkpoint finishes,
+ * the postmaster sends us SIGUSR2. This instructs walsender to send any
+ * outstanding WAL, including the shutdown checkpoint record, wait for it to be
+ * replicated to the standby, and then exit.
*
*
* Portions Copyright (c) 2010-2024, PostgreSQL Global Development Group
--
1.8.3.1
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: walsender.c fileheader comment
@ 2024-07-16 20:56 Tomas Vondra <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Tomas Vondra @ 2024-07-16 20:56 UTC (permalink / raw)
To: Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]>
On 6/11/24 04:35, Peter Smith wrote:
> Hi,
>
> I was reading the walsender.c fileheader comment while studying
> another thread. I think if there is logical replication in progress
> then the PROCSIG_WALSND_INIT_STOPPING handler will *always* switch to
> a "stopping" state: e.g.,
>
> /*
> * Handle PROCSIG_WALSND_INIT_STOPPING signal.
> */
> void
> HandleWalSndInitStopping(void)
> {
> Assert(am_walsender);
>
> /*
> * If replication has not yet started, die like with SIGTERM. If
> * replication is active, only set a flag and wake up the main loop. It
> * will send any outstanding WAL, wait for it to be replicated to the
> * standby, and then exit gracefully.
> */
> if (!replication_active)
> kill(MyProcPid, SIGTERM);
> else
> got_STOPPING = true;
> }
>
> ~~~
>
> But the walsender.c fileheader comment seems to be saying something
> slightly different. IIUC, some minor rewording of the comment is
> needed so it describes the code better.
>
> HEAD
> ...
> * shutdown, if logical replication is in progress all existing WAL records
> * are processed followed by a shutdown. Otherwise this causes the walsender
> * to switch to the "stopping" state. In this state, the walsender will reject
> * any further replication commands. The checkpointer begins the shutdown
> ...
>
> SUGGESTION
> .. shutdown. If logical replication is in progress, the walsender
> switches to a "stopping" state. In this state, the walsender will
> reject any further replication commands - but all existing WAL records
> are processed - followed by a shutdown.
>
> ~~~
>
> I attached a patch for the above-suggested change.
>
> Thoughts?
I did look at this, and while the explanation in the current comment may
seem a bit confusing, I'm not sure the suggested changes improve the
situation very much.
This suggests the two comments somehow disagree, but it does not say in
what exactly, so perhaps I just missed it :-(
ISTM there's a bit of confusion what is meant by "stopping" state - you
seem to be interpreting it as a general concept, where the walsender is
requested to stop (through the signal), and starts doing stuff to exit.
But the comments actually talk about WalSnd->state, where "stopping"
means it needs to be set to WALSNDSTATE_STOPPING.
And we only ever switch to that state in two places - in WalSndPhysical
and exec_replication_command. And that does not happen in regular
logical replication (which is what "logical replication is in progress"
refers to) - if you have a walsender just replicating DML, it will never
see the WALSNDSTATE_STOPPING state. It will simply do the cleanup while
still in WALSNDSTATE_STREAMING state, and then just exit.
So from this point of view, the suggestion is actually wrong.
To conclude, I think this probably makes the comments more confusing. If
we want to make it clearer, I'd probably start by clarifying what the
"stopping" state means. Also, it's a bit surprising we may not actually
go through the "stopping" state during shutdown.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: walsender.c fileheader comment
@ 2024-07-19 05:02 Peter Smith <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Peter Smith @ 2024-07-19 05:02 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi, Thankyou for taking the time to look at this and reply.
>
> I did look at this, and while the explanation in the current comment may
> seem a bit confusing, I'm not sure the suggested changes improve the
> situation very much.
>
> This suggests the two comments somehow disagree, but it does not say in
> what exactly, so perhaps I just missed it :-(
>
> ISTM there's a bit of confusion what is meant by "stopping" state - you
> seem to be interpreting it as a general concept, where the walsender is
> requested to stop (through the signal), and starts doing stuff to exit.
> But the comments actually talk about WalSnd->state, where "stopping"
> means it needs to be set to WALSNDSTATE_STOPPING.
Yes, I interpreted the "stopping" state meaning as when the boolean
flag 'got_STOPPING' is assigned true.
>
> And we only ever switch to that state in two places - in WalSndPhysical
> and exec_replication_command. And that does not happen in regular
> logical replication (which is what "logical replication is in progress"
> refers to) - if you have a walsender just replicating DML, it will never
> see the WALSNDSTATE_STOPPING state. It will simply do the cleanup while
> still in WALSNDSTATE_STREAMING state, and then just exit.
>
> So from this point of view, the suggestion is actually wrong.
OK.
>
> To conclude, I think this probably makes the comments more confusing. If
> we want to make it clearer, I'd probably start by clarifying what the
> "stopping" state means. Also, it's a bit surprising we may not actually
> go through the "stopping" state during shutdown.
>
I agree. My interpretation of the (ambiguous) "stopping" state led me
to believe the comment was quite wrong. So, this thread was only
intended as a trivial comment fix in passing but clearly there is more
to this than I anticipated. I would be happy if someone with more
knowledge about the WALSNDSTATE_STOPPING versus got_STOPPING could
disambiguate the file header comment, but that's not me, so I have
withdrawn this from the Commitfest.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: walsender.c fileheader comment
@ 2024-07-19 13:25 Tomas Vondra <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Tomas Vondra @ 2024-07-19 13:25 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 7/19/24 07:02, Peter Smith wrote:
> ...
>>
>> To conclude, I think this probably makes the comments more confusing. If
>> we want to make it clearer, I'd probably start by clarifying what the
>> "stopping" state means. Also, it's a bit surprising we may not actually
>> go through the "stopping" state during shutdown.
>>
>
> I agree. My interpretation of the (ambiguous) "stopping" state led me
> to believe the comment was quite wrong. So, this thread was only
> intended as a trivial comment fix in passing but clearly there is more
> to this than I anticipated. I would be happy if someone with more
> knowledge about the WALSNDSTATE_STOPPING versus got_STOPPING could
> disambiguate the file header comment, but that's not me, so I have
> withdrawn this from the Commitfest.
>
Understood. Thanks for the patch anyway, I appreciate you took the time
to try to improve the comments!
I agree the state transitions in walsender are not very clear, and the
fact that it may shutdown without ever going through STOPPING state is
quite confusing. That being said, I personally don't have ambition to
improve this.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 27+ messages in thread
end of thread, other threads:[~2024-07-19 13:25 UTC | newest]
Thread overview: 27+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v8 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v10 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2020-12-01 23:20 [PATCH v9 4/4] WIP: check tuple size Justin Pryzby <[email protected]>
2024-06-11 02:35 walsender.c fileheader comment Peter Smith <[email protected]>
2024-07-16 20:56 ` Re: walsender.c fileheader comment Tomas Vondra <[email protected]>
2024-07-19 05:02 ` Re: walsender.c fileheader comment Peter Smith <[email protected]>
2024-07-19 13:25 ` Re: walsender.c fileheader comment Tomas Vondra <[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