On Thu, Apr 22, 2021 at 7:28 PM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > I have refactored the patch to address your other concerns. Breaking the > patch into multiple pieces didn't add any clarity, but refactoring portions > of it made things simpler to read, I think, so here it is as one patch file.
I was hoping that this version was going to be smaller than the last version, but instead it went from 300+ lines to 500+ lines. The main thing I'm unhappy about in the status quo is the use of chunkno in error messages. I have suggested several times making that concept go away, because I think users will be confused. Here's a minimal patch that does just that. It's 32 lines and results in a net removal of 4 lines. It differs somewhat from my earlier suggestions, because my priority here is to get reasonably understandable output without needing a ton of code, and as I was working on this I found that some of my earlier suggestions would have needed more code to implement and I didn't think it bought enough to be worth it. It's possible this is too simple, or that it's buggy, so let me know what you think. But basically, I think what got committed before is actually mostly fine and doesn't need major revision. It just needs tidying up to avoid the confusing chunkno concept. Now, the other thing we've talked about is adding a few more checks, to verify for example that the toastrelid is what we expect, and I see in your v22 you thought of a few other things. I think we can consider those, possibly as things where we consider it tidying up loose ends for v14, or else as improvements for v15. But I don't think that the fairly large size of your patch comes primarily from additional checks. I think it mostly comes from the code to produce error reports getting a lot more complicated. I apologize if my comments have driven that complexity, but they weren't intended to. One tiny problem with the attached patch is that it does not make any regression tests fail, which also makes it hard for me to tell if it breaks anything, or if the existing code works. I don't know how practical it is to do anything about that. Do you have a patch handy that allows manual updates and deletes on TOAST tables, for manual testing purposes? -- Robert Haas EDB: http://www.enterprisedb.com
simply-remove-chunkno-concept.patch
Description: Binary data
