On Thu, Apr 06, 2023 at 10:54:43AM +0900, Michael Paquier wrote: > 0002 can also be done before 0001, so I'd like to get that part > applied on HEAD before the feature freeze and close this thread. If > there are any objections, please feel free..
I was doing a pre-commit review of the patch, and double-checked the uses of mainrdata_len. And there is this part: /* followed by main data, if any */ if (mainrdata_len > 0) { if (mainrdata_len > 255) { *(scratch++) = (char) XLR_BLOCK_ID_DATA_LONG; memcpy(scratch, &mainrdata_len, sizeof(uint32)); scratch += sizeof(uint32); } else { *(scratch++) = (char) XLR_BLOCK_ID_DATA_SHORT; *(scratch++) = (uint8) mainrdata_len; } rdt_datas_last->next = mainrdata_head; rdt_datas_last = mainrdata_last; total_len += mainrdata_len; } rdt_datas_last->next = NULL; So bumping mainrdata_len to uint64 is actually not entirely in line with this code. Well, it will work because we'd still fail a couple of lines down, but perhaps its readability should be improved so as we have an extra check in this code path to make sure that mainrdata_len is not higher than PG_UINT32_MAX, then use an intermediate casted variable before saving the length in the record data to make clear that the type of the main static length in xloginsert.c is not the same as what a record has? The v10 I sent previously blocked this possibility, but not v11. -- Michael
signature.asc
Description: PGP signature