Re: WIP: Access method extendability - Mailing list pgsql-hackers

<div dir="ltr"><div class="gmail_extra"><div class="gmail_extra">Hi!</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Thankseverybody for discussion. Sorry for delay. I'll try to address most of questions arised in
thisdiscussion.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">In general, I'm agree with concept
ofmarking index as invalid in certain cases.</div><div class="gmail_extra">I see following possible consequences of
buggyWAL-logged custom AM:</div><div class="gmail_extra">1) Server crash during insert/update.</div><div
class="gmail_extra">2)Server crash during select. </div><div class="gmail_extra">3) Invalid query answers.</div><div
class="gmail_extra">4)Server crash during vacuum.</div><div class="gmail_extra">5) Server crash in recovery.</div><div
class="gmail_extra"><br/></div><div class="gmail_extra">#5 is totally unacceptable. I've tried to design generic WAL
recordso that it should be always possible to purely mechanically apply the record. It's always possible to move piece
ofmemory inside the page. It's always possible to copy piece of memory from WAL-record to the page. Buggy WAL for sure
couldcause an index corruption as well as any other bug in AM. WAL support doesn't bring nothing special to this expect
thefact that WAL is quite hard to debug.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">However, in
currentimplementation I missed some hidden asserts about page structure. Page could be so corrupted that we can't, for
instance,safely call XLogReadBufferForRedo(). All this cases must be worked out. If we can't update page during
recovery,index must be marked as invalid. It's some amount of work, but it seems to be very feasible.</div><div
class="gmail_extra"><br/></div><div class="gmail_extra">#4 seems problematic too. If autovacuum crashes on some index,
thenpostgres can enter a crash loop. So, if autovacuum crashes on extension provided AM, that index should be marked as
invalid.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Consequences #1, #3 and #3 could be easily
causedby buggy opclass as well. The reason why we're not knee-deep in extension provied bugs in GiST/GIN opclasses is
noteasyness of writing correct GiST/GIN opclasses. Actual reason is that we have only few GiST/GIN opclasses which
weren'twritten by GiST/GIN authors.  </div><div class="gmail_extra"><br /></div><div class="gmail_extra">So, I don't
expectPostgreSQL to be flooded with buggy AMs once we add AM extendability. Our motivation behind this proposal is that
wewant to be able to develop custom extensions with AMs. We want to be able to provide our AMs to our customers
whithouthaving to push that into PostgreSQL core or fork PostgreSQL. Bugs in that AMs in our responsibility to out
customers.Some commercial companies could implement patented AMs (for instance, fractal tree), and that is their
responsibilityto their customers. </div><div class="gmail_extra"><br /></div><div class="gmail_extra">Also, I think
it'sOK to put adopting custom AMs to changes of AM interface to authors of those custom AMs. AM extensions anyway
shouldbe adopted to each new major release. AFAIR, interface of RelationOpen() function has been changed not too long
ago.Custom AM would use many functions which we use to access relations. Their interface can be changed in the next
release.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">PostGIS GiST opclass has bugs which are
reproducable,known and not fixed. This is responsibility of PostGIS to their customers. We can feel sorry for PostGIS
thatthey are so slow on fixing this. But we shouldn't feel sorry for GiST extendability, that woulde be
redicilous.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Some recearches could write their own
extensions.We can hope that they are smart enough to not recommend it for production use. We can back our hope with
warningduring installing extension provided AM. That warning could say that all corruption caused by extension provided
AMis up to AM developer. This warning could make users to beware of using extension provided AMs in
production </div><divclass="gmail_extra">unless they are fully trust extension developer (have support subscription if
it'scommercial).</div><div class="gmail_extra"><br /></div><div class="gmail_extra">PostgreSQL doesn't have any kind of
safeextensions. Every extension must be trusted. Every extension can break (almost) everything.When one types CREATE
EXTENSIONhe must completely trust extension author. This applies to every extension. </div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">I would be very careful with discouraging commercial AM extensions. We should always
keenin the mind how many of PostgreSQL developers are working for companies which own their commercial PostgreSQL forks
andhow big their contribution is. Patented extensions looks scary for sure. But it's up to software patents not up to
PostgreSQLextendability...</div><div class="gmail_extra"><br /></div><div class="gmail_extra">Particular steps I'm
goingto do on these patches:</div><div class="gmail_extra">1) Make generic_redo never crash on broken pages.</div><div
class="gmail_extra">2)Make autovacuum launcher mark index as invalid if vacuum process crashed on custom AM index.
Since,we can't write something into postgres cluster when one process has crushed, ITSM autovacuum should have some
separateplace to put this information. Thus, after restart postgres can read it and mark index as invalid.</div><div
class="gmail_extra"><br/></div><div class="gmail_extra">Don't allowing CREATE ACCESS METHOD command seems problematic
forme. How could it work with pg_upgrade? pg_dump wouldn't dump extra pg_am records. So, pg_upgrade would break at
creatingoperator classes on new cluster. So, I agree with dropping create am command only if we let pg_dump to dump
extrapg_am records...</div><div class="gmail_extra"><br /></div>------<br />With best regards,<br />Alexander
Korotkov.</div></div>