8000 Don't try to print data type names in slot_store_error_callback(). · postgres/postgres@0b5089e · GitHub
[go: up one dir, main page]
Skip to content

Commit 0b5089e

Browse files
tglsfdctglsfdc
committed
Don't try to print data type names in slot_store_error_callback().
The existing code tried to do syscache lookups in an already-failed transaction, which is problematic to say the least. After some consideration of alternatives, the best fix seems to be to just drop type names from the error message altogether. The table and column names seem like sufficient localization. If the user is unsure what types are involved, she can check the local and remote table definitions. Having done that, we can also discard the LogicalRepTypMap hash table, which had no other use. Arguably, LOGICAL_REP_MSG_TYPE replication messages are now obsolete as well; but we should probably keep them in case some other use emerges. (The complexity of removing something from the replication protocol would likely outweigh any savings anyhow.) Masahiko Sawada and Bharath Rupireddy, per complaint from Andres Freund. Back-patch to v10 where this code originated. Discussion: https://postgr.es/m/20210106020229.ne5xnuu6wlondjpe@alap3.anarazel.de
1 parent 177531e commit 0b5089e

File tree

3 files changed

+6
-133
lines changed

3 files changed

+6
-133
lines changed

‎src/backend/replication/logical/relation.c

Lines changed: 1 addition & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,13 @@
2424
#include "nodes/makefuncs.h"
2525
#include "replication/logicalrelation.h"
2626
#include "replication/worker_internal.h"
27-
#include "utils/builtins.h"
2827
#include "utils/inval.h"
29-
#include "utils/lsyscache.h"
3028
#include "utils/memutils.h"
31-
#include "utils/syscache.h"
29+
3230

3331
static MemoryContext LogicalRepRelMapContext = NULL;
3432

3533
static HTAB *LogicalRepRelMap = NULL;
36-
static HTAB *LogicalRepTypMap = NULL;
3734

3835

3936
/*
@@ -100,16 +97,6 @@ logicalrep_relmap_init(void)
10097
LogicalRepRelMap = hash_create("logicalrep relation map cache", 128, &ctl,
10198
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
10299

103-
/* Initialize the type hash table. */
104-
MemSet(&ctl, 0, sizeof(ctl));
105-
ctl.keysize = sizeof(Oid);
106-
ctl.entrysize = sizeof(LogicalRepTyp);
107-
ctl.hcxt = LogicalRepRelMapContext;
108-
109-
/* This will usually be small. */
110-
LogicalRepTypMap = hash_create("logicalrep type map cache", 2, &ctl,
111-
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
112-
113100
/* Watch for invalidation events. */
114101
CacheRegisterRelcacheCallback(logicalrep_relmap_invalidate_cb,
115102
(Datum) 0);
@@ -397,92 +384,3 @@ logicalrep_rel_close(LogicalRepRelMapEntry *rel, LOCKMODE lockmode)
397384
heap_close(rel->localrel, lockmode);
398385
rel->localrel = NULL;
399386
}
400-
401-
/*
402-
* Free the type map cache entry data.
403-
*/
404-
static void
405-
logicalrep_typmap_free_entry(LogicalRepTyp *entry)
406-
{
407-
pfree(entry->nspname);
408-
pfree(entry->typname);
409-
}
410-
411-
/*
412-
* Add new entry or update existing entry in the type map cache.
413-
*/
414-
void
415-
logicalrep_typmap_update(LogicalRepTyp *remotetyp)
416-
{
417-
MemoryContext oldctx;
418-
LogicalRepTyp *entry;
419-
bool found;
420-
421-
if (LogicalRepTypMap == NULL)
422-
logicalrep_relmap_init();
423-
424-
/*
425-
* HASH_ENTER returns the existing entry if present or creates a new one.
426-
*/
427-
entry = hash_search(LogicalRepTypMap, (void *) &remotetyp->remoteid,
428-
HASH_ENTER, &found);
429-
430-
if (found)
431-
logicalrep_typmap_free_entry(entry);
432-
433-
/* Make cached copy of the data */
434-
entry->remoteid = remotetyp->remoteid;
435-
oldctx = MemoryContextSwitchTo(LogicalRepRelMapContext);
436-
entry->nspname = pstrdup(remotetyp->nspname);
437-
entry->typname = pstrdup(remotetyp->typname);
438-
MemoryContextSwitchTo(oldctx);
439-
}
440-
441-
/*
442-
* Fetch type name from the cache by remote type OID.
443-
*
444-
* Return a substitute value if we cannot find the data type; no message is
445-
* sent to the log in that case, because this is used by error callback
446-
* already.
447-
*/
448-
char *
449-
logicalrep_typmap_gettypname(Oid remoteid)
450-
{
451-
LogicalRepTyp *entry;
452-
bool found;
453-
454-
/* Internal types are mapped directly. */
455-
if (remoteid < FirstBootstrapObjectId)
456-
{
457-
if (!get_typisdefined(remoteid))
458-
{
459-
/*
460-
* This can be caused by having a publisher with a higher
461-
* PostgreSQL major version than the subscriber.
462-
*/
463-
return psprintf("unrecognized %u", remoteid);
464-
}
465-
466-
return format_type_be(remoteid);
467-
}
468-
469-
if (LogicalRepTypMap == NULL)
470-
{
471-
/*
472-
* If the typemap is not initialized yet, we cannot possibly attempt
473-
* to search the hash table; but there's no way we know the type
474-
* locally yet, since we haven't received a message about this type,
475-
* so this is the best we can do.
476-
*/
477-
return psprintf("unrecognized %u", remoteid);
478-
}
479-
480-
/* search the mapping */
481-
entry = hash_search(LogicalRepTypMap, (void *) &remoteid,
482-
HASH_FIND, &found);
483-
if (!found)
484-
return psprintf("unrecognized %u", remoteid);
485-
486-
Assert(OidIsValid(entry->remoteid));
487-
return psprintf("%s.%s", entry->nspname, entry->typname);
488-
}

‎src/backend/replication/logical/worker.c

Lines changed: 5 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ static dlist_head lsn_mapping = DLIST_STATIC_INIT(lsn_mapping);
102102
typedef struct SlotErrCallbackArg
103103
{
104104
LogicalRepRelMapEntry *rel;
105-
int local_attnum;
106105
int remote_attnum;
107106
} SlotErrCallbackArg;
108107

@@ -278,36 +277,23 @@ slot_fill_defaults(LogicalRepRelMapEntry *rel, EState *estate,
278277
}
279278

280279
/*
281-
* Error callback to give more context info about type conversion failure.
280+
* Error callback to give more context info about data conversion failures
281+
* while reading data from the remote server.
282282
*/
283283
static void
284284
slot_store_error_callback(void *arg)
285285
{
286286
SlotErrCallbackArg *errarg = (SlotErrCallbackArg *) arg;
287287
LogicalRepRelMapEntry *rel;
288-
char *remotetypname;
289-
Oid remotetypoid,
290-
localtypoid;
291288

292289
/* Nothing to do if remote attribute number is not set */
293290
if (errarg->remote_attnum < 0)
294291
return;
295292

296293
rel = errarg->rel;
297-
remotetypoid = rel->remoterel.atttyps[errarg->remote_attnum];
298-
299-
/* Fetch remote type name from the LogicalRepTypMap cache */
300-
remotetypname = logicalrep_typmap_gettypname(remotetypoid);
301-
302-
/* Fetch local type OID from the local sys cache */
303-
localtypoid = get_atttype(rel->localreloid, errarg->local_attnum + 1);
304-
305-
errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\", "
306-
"remote type %s, local type %s",
294+
errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\"",
307295
rel->remoterel.nspname, rel->remoterel.relname,
308-
rel->remoterel.attnames[errarg->remote_attnum],
309-
remotetypname,
310-
format_type_be(localtypoid));
296+
rel->remoterel.attnames[errarg->remote_attnum]);
311297
}
312298

313299
/*
@@ -328,7 +314,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
328314

329315
/* Push callback + info on the error context stack */
330316
errarg.rel = rel;
331-
errarg.local_attnum = -1;
332317
errarg.remote_attnum = -1;
333318
errcallback.callback = slot_store_error_callback;
334319
errcallback.arg = (void *) &errarg;
@@ -347,7 +332,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
347332
Oid typinput;
348333
Oid typioparam;
349334

350-
errarg.local_attnum = i;
351335
errarg.remote_attnum = remoteattnum;
352336

353337
getTypeInputInfo(att->atttypid, &typinput, &typioparam);
@@ -356,7 +340,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
356340
typioparam, att->atttypmod);
357341
slot->tts_isnull[i] = false;
358342

359-
errarg.local_attnum = -1;
360343
errarg.remote_attnum = -1;
361344
}
362345
else
@@ -412,7 +395,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
412395

413396
/* For error reporting, push callback + info on the error context stack */
414397
errarg.rel = rel;
415-
errarg.local_attnum = -1;
416398
errarg.remote_attnum = -1;
417399
errcallback.callback = slot_store_error_callback;
418400
errcallback.arg = (void *) &errarg;
@@ -436,7 +418,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
436418
Oid typinput;
437419
Oid typioparam;
438420

439-
errarg.local_attnum = i;
440421
errarg.remote_attnum = remoteattnum;
441422

442423
getTypeInputInfo(att->atttypid, &typinput, &typioparam);
@@ -445,7 +426,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
445426
typioparam, att->atttypmod);
446427
slot->tts_isnull[i] = false;
447428

448-
errarg.local_attnum = -1;
449429
errarg.remote_attnum = -1;
450430
}
451431
else
@@ -569,16 +549,14 @@ apply_handle_relation(StringInfo s)
569549
/*
570550
* Handle TYPE message.
571551
*
572-
* Note we don't do local mapping here, that's done when the type is
573-
* actually used.
552+
* This is now vestigial; we read the info and discard it.
574553
*/
575554
static void
576555
apply_handle_type(StringInfo s)
577556
{
578557
LogicalRepTyp typ;
579558

580559
logicalrep_read_typ(s, &typ);
581-
logicalrep_typmap_update(&typ);
582560
}
583561

584562
/*

‎src/include/replication/logicalrelation.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,4 @@ extern LogicalRepRelMapEntry *logicalrep_rel_open(LogicalRepRelId remoteid,
3838
extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
3939
LOCKMODE lockmode);
4040

41-
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
42-
extern char *logicalrep_typmap_gettypname(Oid remoteid);
43-
4441
#endif /* LOGICALRELATION_H */

0 commit comments

Comments
 (0)
0