On Wed, Oct 7, 2020 at 5:00 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Wed, Oct 7, 2020 at 10:26 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Tue, Oct 6, 2020 at 10:21 PM Tomas Vondra > > <tomas.von...@2ndquadrant.com> wrote: > > > > > > On Tue, Oct 06, 2020 at 11:00:55AM +0530, Dilip Kumar wrote: > > > >On Mon, Oct 5, 2020 at 9:34 PM Tomas Vondra > > > ><tomas.von...@2ndquadrant.com> wrote: > > > >> > > > >> On Mon, Oct 05, 2020 at 07:57:41PM +0530, Dilip Kumar wrote: > > > >> >On Mon, Oct 5, 2020 at 5:53 PM Tomas Vondra > > > >> ><tomas.von...@2ndquadrant.com> wrote: > > > >> >> > > > >> >> On Mon, Oct 05, 2020 at 11:17:28AM +0530, Dilip Kumar wrote: > > > >> >> >On Mon, Oct 5, 2020 at 3:37 AM Tomas Vondra > > > >> >> ><tomas.von...@2ndquadrant.com> wrote: > > > >> >> > > > > >> >> >Thanks, Tomas for your feedback. > > > >> >> > > > > >> >> >> 9) attcompression ... > > > >> >> >> > > > >> >> >> The main issue I see is what the patch does with attcompression. > > > >> >> >> Instead > > > >> >> >> of just using it to store a the compression method, it's also > > > >> >> >> used to > > > >> >> >> store the preserved compression methods. And using NameData to > > > >> >> >> store > > > >> >> >> this seems wrong too - if we really want to store this info, the > > > >> >> >> correct > > > >> >> >> way is either using text[] or inventing charvector or similar. > > > >> >> > > > > >> >> >The reason for using the NameData is the get it in the fixed part > > > >> >> >of > > > >> >> >the data structure. > > > >> >> > > > > >> >> > > > >> >> Why do we need that? It's possible to have varlena fields with > > > >> >> direct > > > >> >> access (see pg_index.indkey for example). > > > >> > > > > >> >I see. While making it NameData I was thinking whether we have an > > > >> >option to direct access the varlena. Thanks for pointing me there. I > > > >> >will change this. > > > >> > > > > >> > Adding NameData just to make > > > >> >> it fixed-length means we're always adding 64B even if we just need a > > > >> >> single byte, which means ~30% overhead for the > > > >> >> FormData_pg_attribute. > > > >> >> That seems a bit unnecessary, and might be an issue with many > > > >> >> attributes > > > >> >> (e.g. with many temp tables, etc.). > > > >> > > > > >> >You are right. Even I did not like to keep 64B for this, so I will > > > >> >change it. > > > >> > > > > >> >> > > > >> >> >> But to me this seems very much like a misuse of attcompression > > > >> >> >> to track > > > >> >> >> dependencies on compression methods, necessary because we don't > > > >> >> >> have a > > > >> >> >> separate catalog listing compression methods. If we had that, I > > > >> >> >> think we > > > >> >> >> could simply add dependencies between attributes and that > > > >> >> >> catalog. > > > >> >> > > > > >> >> >Basically, up to this patch, we are having only built-in > > > >> >> >compression > > > >> >> >methods and those can not be dropped so we don't need any > > > >> >> >dependency > > > >> >> >at all. We just want to know what is the current compression > > > >> >> >method > > > >> >> >and what is the preserve compression methods supported for this > > > >> >> >attribute. Maybe we can do it better instead of using the NameData > > > >> >> >but I don't think it makes sense to add a separate catalog? > > > >> >> > > > > >> >> > > > >> >> Sure, I understand what the goal was - all I'm saying is that it > > > >> >> looks > > > >> >> very much like a workaround needed because we don't have the > > > >> >> catalog. > > > >> >> > > > >> >> I don't quite understand how could we support custom compression > > > >> >> methods > > > >> >> without listing them in some sort of catalog? > > > >> > > > > >> >Yeah for supporting custom compression we need some catalog. > > > >> > > > > >> >> >> Moreover, having the catalog would allow adding compression > > > >> >> >> methods > > > >> >> >> (from extensions etc) instead of just having a list of hard-coded > > > >> >> >> compression methods. Which seems like a strange limitation, > > > >> >> >> considering > > > >> >> >> this thread is called "custom compression methods". > > > >> >> > > > > >> >> >I think I forgot to mention while submitting the previous patch > > > >> >> >that > > > >> >> >the next patch I am planning to submit is, Support creating the > > > >> >> >custom > > > >> >> >compression methods wherein we can use pg_am catalog to insert the > > > >> >> >new > > > >> >> >compression method. And for dependency handling, we can create an > > > >> >> >attribute dependency on the pg_am row. Basically, we will create > > > >> >> >the > > > >> >> >attribute dependency on the current compression method AM as well > > > >> >> >as > > > >> >> >on the preserved compression methods AM. As part of this, we will > > > >> >> >add two build-in AMs for zlib and pglz, and the attcompression > > > >> >> >field > > > >> >> >will be converted to the oid_vector (first OID will be of the > > > >> >> >current > > > >> >> >compression method, followed by the preserved compression method's > > > >> >> >oids). > > > >> >> > > > > >> >> > > > >> >> Hmmm, ok. Not sure pg_am is the right place - compression methods > > > >> >> don't > > > >> >> quite match what I though AMs are about, but maybe it's just my > > > >> >> fault. > > > >> >> > > > >> >> FWIW it seems a bit strange to first do the attcompression magic and > > > >> >> then add the catalog later - I think we should start with the > > > >> >> catalog > > > >> >> right away. The advantage is that if we end up committing only some > > > >> >> of > > > >> >> the patches in this cycle, we already have all the infrastructure > > > >> >> etc. > > > >> >> We can reorder that later, though. > > > >> > > > > >> >Hmm, yeah we can do this way as well that first create a new catalog > > > >> >table and add entries for these two built-in methods and the > > > >> >attcompression can store the oid vector. But if we only commit the > > > >> >build-in compression methods part then does it make sense to create an > > > >> >extra catalog or adding these build-in methods to the existing catalog > > > >> >(if we plan to use pg_am). Then in attcompression instead of using > > > >> >one byte for each preserve compression method, we need to use oid. So > > > >> >from Robert's mail[1], it appeared to me that he wants that the > > > >> >build-in compression methods part should be independently committable > > > >> >and if we think from that perspective then adding a catalog doesn't > > > >> >make much sense. But if we are planning to commit the custom method > > > >> >also then it makes more sense to directly start with the catalog > > > >> >because that way it will be easy to expand without much refactoring. > > > >> > > > > >> >[1] > > > >> >https://www.postgresql.org/message-id/CA%2BTgmobSDVgUage9qQ5P_%3DF_9jaMkCgyKxUQGtFQU7oN4kX-AA%40mail.gmail.com > > > >> > > > > >> > > > >> Hmmm. Maybe I'm missing something subtle, but I think that plan can be > > > >> interpreted in various ways - it does not really say whether the > > > >> initial > > > >> list of built-in methods should be in some C array, or already in a > > > >> proper > > > >> catalog. > > > >> > > > >> All I'm saying is it seems a bit weird to first implement dependencies > > > >> based on strange (mis)use of attcompression attribute, and then replace > > > >> it with a proper catalog. My understanding is those patches are > > > >> expected > > > >> to be committable one by one, but the attcompression approach seems a > > > >> bit too hacky to me - not sure I'd want to commit that ... > > > > > > > >Okay, I will change this. So I will make create a new catalog > > > >pg_compression and add the entry for two built-in compression methods > > > >from the very first patch. > > > > > > > > > > OK. > > I have changed the first 2 patches, basically, now we are providing a > new catalog pg_compression and the pg_attribute is storing the oid of > the compression method. The patches still need some cleanup and there > is also one open comment that for index we should use its table > compression. > > I am still working on the preserve patch. For preserving the > compression method I am planning to convert the attcompression field > to the oidvector so that we can store the oid of the preserve method > also. I am not sure whether we can access this oidvector as a fixed > part of the FormData_pg_attribute or not. The reason is that for > building the tuple descriptor, we need to give the size of the fixed > part (#define ATTRIBUTE_FIXED_PART_SIZE \ > (offsetof(FormData_pg_attribute,attcompression) + sizeof(Oid))). But > if we convert this to the oidvector then we don't know the size of the > fixed part. Am I missing something?
I could think of two solutions here Sol1. Make the first oid of the oidvector as part of the fixed size, like below #define ATTRIBUTE_FIXED_PART_SIZE \ (offsetof(FormData_pg_attribute, attcompression) + OidVectorSize(1)) Sol2: Keep attcompression as oid only and for the preserve list, adds another field in the variable part which will be of type oidvector. I think most of the time we need to access the current compression method and with this solution, we will be able to access that as part of the tuple desc. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
